GODRIVER-1898 SDAM error handling changes for LB mode #611
Conversation
divjotarora
force-pushed
the
godriver1898-lb-sdam-errors
branch
from
a2b058a
to
d7f0cb3
Compare
divjotarora
force-pushed
the
godriver1898-lb-sdam-errors
branch
from
d7f0cb3
to
5018113
Compare
| // Ignore the error if the server is behind a load balancer but the server ID is unknown. This indicates that the | ||
| // error happened when dialing the connection or during the MongoDB handshake, so we don't know the server ID to use | ||
| // for clearing the pool. | ||
| if err == nil || s.cfg.loadBalanced && serverID == nil { |
There was a problem hiding this comment.
IIUC, it should be impossible for serverID to be primitive.NilObjectID when in load balanced mode. In load balanced mode, any handshake that omits a serverID in the reply will result in an error.
There was a problem hiding this comment.
Note that there's a distinction between nil and primitive.NilObjectID. The former is a nil pointer and the latter is a non-nil pointer that contains 12 0 bytes. In LB mode, the former is possible when we get here if we encountered an error before the initial hello succeeded. The latter is never possible because 12 0 bytes is not a valid ObjectId (at least not right now because the timestamp field would be non-zero).
| // setGenerationNumber sets the connection's generation number if a callback has been provided to do so in connection | ||
| // configuration. | ||
| func (c *connection) setGenerationNumber() { | ||
| if c.config.getGenerationFn != nil { |
There was a problem hiding this comment.
Would getGenerationFn ever be nil? The serverID => (generation, count) map is used to determine generation regardless of whether we are in load balanced mode.
There was a problem hiding this comment.
For unit tests, it can be. I verified that removing the line in pool.go that sets this function causes some of the new spec tests to fail though.
| @@ -288,7 +294,7 @@ func (s *Server) ProcessHandshakeError(err error, startingGenerationNumber uint6 | |||
| // the description.Server appropriately. The description should not have a TopologyVersion because the staleness | |||
| // checking logic above has already determined that this description is not stale. | |||
| s.updateDescription(description.NewServerFromError(s.address, wrappedConnErr, nil)) | |||
There was a problem hiding this comment.
To check, would updating the server description to unknown make it ineligible for server selection (and similarly in ProcessError?
There was a problem hiding this comment.
The spec actually prohibits us from marking the server Unknown to ensure that the LB is always selectable. As part of a previous PR in this epic, I changed updateDescription to be a no-op when the server type is LoadBalancer. I felt this was a cleaner approach than checking every invocation.
There was a problem hiding this comment.
Ah right, that makes sense. Thanks for the explanations!
Co-authored-by: Kevin Albertson <kevin.albertson@10gen.com>
Summary of changes:
uint64, use a map fromserverID(which is an ObjectID) to a tuple of(generation number, connection count). In this new map type,primitive.NilObjectIDis used to store the generation and connection count for servers that do not return aserverIdfield inismasterresponses (i.e. servers in non-load balanced deployments).ServerIDfield toPoolClearedevents.