Attempt to connect to leader via 10 servers simultaneously #292
Conversation
internal/protocol/connector.go
Outdated
|
|
||
| ctx, cancel := context.WithTimeout(ctx, c.config.AttemptTimeout) | ||
| defer cancel() | ||
| sem.Acquire(ctx, 1) |
There was a problem hiding this comment.
This can return an error. We should handle that case, and if you release too many times, it will cause a panic. We should be careful in that scenario.
There was a problem hiding this comment.
Updated, following the example
There was a problem hiding this comment.
There is still the case where the semaphore can be acquired after cancellation. To sacrifice performance but improve readability we could also eliminate (new) lines 156-158
There was a problem hiding this comment.
We do exactly the same in juju 😉
internal/protocol/connector.go
Outdated
| }(server, protocolChan) | ||
| } | ||
|
|
||
| // Read from protocol chan, cancel context\ |
internal/protocol/connector.go
Outdated
| var protocol *Protocol | ||
| for pc := range protocolChan { | ||
| if protocol != nil { | ||
| pc.Close() | ||
| continue | ||
| } | ||
| log(logging.Debug, "connected") | ||
| protocol = pc | ||
| cancel() | ||
| } | ||
|
|
||
| if protocol != nil { |
There was a problem hiding this comment.
I feel this would be a bit clearer as follows:
protocol, ok := <-protocolChan
if !ok {
return nil, ErrNoAvailableLeader
}
cancel()
for extra := range protocolChan {
extra.Close()
}
return protocol, nil
|
@cole-miller would you like me to squash these commits to keep the history clean? |
|
I don't think it's necessary! |
|
🚀 woohoo! 🚀 |
This PR implements the solution discussed in my first PR
a. Grab semaphore, defer release
b. Check if a connection has been found, exit
c. Execute original logic, write protocol output to channel
Resolves #134