PYTHON-2674 Pool.reset only clears connections to the given serviceId #628
Conversation
Maintain individual generations for each serviceId.
|
I just had an idea which simplify the _PoolGeneration class a good amount. Please hold off reviewing this until I update. |
pymongo/pool.py
Outdated
|
|
||
| def get_overall(self): | ||
| """Get the Pool's overall generation.""" | ||
| return self._generations[None] |
There was a problem hiding this comment.
The simplification I was going to make was to combine the get and get_overall methods since get(None) is equivalent to get_overall() but I decided that actually makes for more cryptic code. Instead I think we should keep them separate. get_overall() is called to get the "overall pool generation" before a connection is created. get(service_id) is called after a service_id is known (eg in SocketInfo._ismaster and Pool.stale_generation).
There was a problem hiding this comment.
When we are in LB mode what is the significance of the overall generation ID? Aren't generation numbers only relevant on a per-service ID basis in that case?
There was a problem hiding this comment.
It's used in 2 places when in LB mode:
- First, when an error occurs before the handshake completes, the service_id will be None. According to the spec, this means that we have to clear all the connections in the pool.
- Second, the "overall" generation is used to determine if the pool has been cleared in the "remove_stale_sockets" background job.
The second one is when it's relevant to keep track of the total number of times the pool has been cleared (which is essentially what self._generations[None] tracks).
pymongo/pool.py
Outdated
|
|
||
| def get_overall(self): | ||
| """Get the Pool's overall generation.""" | ||
| return self._generations[None] |
There was a problem hiding this comment.
When we are in LB mode what is the significance of the overall generation ID? Aren't generation numbers only relevant on a per-service ID basis in that case?
pymongo/pool.py
Outdated
| # Maps service_id to generation. | ||
| self._generations = collections.defaultdict(int) | ||
| # Overall pool generation. | ||
| self._generations[None] = 0 |
There was a problem hiding this comment.
Can we use a better sentinel than None for tracking overall generation ID? Does this even need to be in the _generations map? I think we can just track it as an integer class attribute.
There was a problem hiding this comment.
Done, what do you think?
Maintain individual generations for each serviceId. Note that new tests for this change (to exercise the LB serviceId code path) depend on connection pinning and have been added in #630