fix(core): allow requests to be queued in CONNECTING state (#374) [v2] #588
Conversation
kazoo/tests/test_connection.py
Outdated
| _create_connection = handler.create_connection | ||
|
|
||
| def controlled_create_connection(*args, **kwargs): | ||
| ev_can_connect.wait(5) |
There was a problem hiding this comment.
This feels like a really involved test setup. Would you not be able to reproduce the same with
- ZkClient configured with infinite retries, flat backoff, zero jitter
- ZkServer initially downed.
- You start the client -> it enters the connecting state as it keeps trying to connect
- You can submit commands -> no exceptions (with your patch)
- You start the server -> the commands should be effected.
I think it would make the test case less intrusive.
What do you think?
There was a problem hiding this comment.
Argh :) What you are suggesting is close to what the previous test was doing:
https://github.com/python-zk/kazoo/pull/583/files#diff-b526bcbf3d9f758b2032e6763cb453c7R601
(Except for the "starting with ZK down" bit, which would indeed make things easier. I don't know if any other tests start/stop ZK, though; I will check.)
I have switched because test_connection_write_timeout, which was already using a pretty intrusive method, was close to what I wanted to test—and I thought that making a single test a bit more complicated was the lesser evil.
The new test_connection_lost_empties_queue test, which is largely a duplicate of test_connection_write_timeout, also verifies that the queue empties out when the session cannot be recovered. Would you know of a trick to invalidate the session without being intrusive? (Besides submitting _SESSION_EXPIRED, which is inapplicable because it goes through the queue.)
I must admit that the thing which bothers me more in this test file is the duplication, which I made even worse; I was thinking of factoring out at least some of these local functions, and to submit a subsequent PR if the result turned out to be cleaner.
There was a problem hiding this comment.
Hum, how about starting the client with a random ID, of course unknown to the server. Wouldn't that trigger a session expired error and clear the queues?
As for server up/down, I think I saw this in the tests pertaining to failovers. I might be wrong, would need a minute to go through the tests with a PC
There was a problem hiding this comment.
Okay—I have now moved by tests back to TestClient, and applied some of your suggestions:
Starting with ZK down is not easy, due to the way ZookeeperCluster the test suite interact, and the fact that teardown_zookeeper does not tear down the cluster :) Moreover, the first state change a client perceives is CONNECTED; it never goes from the initial LOST to SUSPENDED.
So instead, I create a fresh client which is backed by a single server, and stop the latter to force a transition to SUSPENDED.
As for flushing: fudging the session password before reconnect is indeed a good way to trigger SessionExpiredError. (Surprisingly enough, just invoking KazooClient._reset_session isn't: a new session gets created, but the state directly transitions from SUSPENDED to CONNECTED!)
Moving the two tests back to TestClient means that they are also exercised under the gevent and eventlet handlers.
…#374) With this patch, requests issued while the client is in the 'CONNECTING' state get queued instead of raising a misleading 'SessionExpiredError'. This fixes python-zk#374, and brings Kazoo more in line with the Java and C clients. See the 'kazoo.client.KazooClient.state' documentation as well as these discussions for more details: python-zk#570 (comment) python-zk#583 (comment)
ztzg
force-pushed
the
fix-374-queue-requests-while-connecting-v2
branch
from
d866316 to
3d68e84
Compare
|
Hi @jeffwidman, @StephenSorriaux: Mind if I nag you a bit about this? :) |
|
Fantastic! Thank you. Now, if I may take this opportunity to bother you a bit more... :) Do you have any plans regarding cutting a new release? And in any case: is there anything I could do which would help? |
|
I think @StephenSorriaux is planning to cut a new release soon. Thank you again for your hard work on this! Only just now looking at this, as I had a deadline at work that kept me busy the past few weeks so had to defer some of my open source involvement for a little bit. |
|
Great! (And no worries; I understand that this is not your full-time occupation :) |
With this patch, requests issued while the client is in the
CONNECTINGstate get queued instead of raising a misleadingSessionExpiredError.This fixes #374, and brings Kazoo more in line with the Java and C clients.
See the
kazoo.client.KazooClient.statedocumentation as well as these discussions for more details:This "v2" is a respin of #583 which only includes the fix for #374.
A few notes:
CONNECTINGstate;(Feel free to suggest a better place for that documentation. I haven't updated
CHANGES.mdas that seems to be done at release time.)