Let the client keep the session open when closing #435
Conversation
The use case is being able to restart a process (e.g. software upgrade) and not losing the ephemeral nodes. The process can save the session id and password in a file, reload them at boot time, and load the kazoo client with the previous session id.
|
Following up on #409. I'm not sure of what needs to be done with regards to the documentation, the existing one for the client looks vastly outdated, and it doesn't feel appropriate to document all of the arguments at once. :) |
|
The CI failures look unrelated to this PR: |
| close_connection = self._send_request( | ||
| read_timeout, | ||
| connect_timeout, | ||
| ) == SESSION_KEPT |
There was a problem hiding this comment.
This feels a bit awkward, tacking on a boolean eval at the end. Can it be changed to get the response first as was done immediately above it?
|
I understand the use-case, I think the code/test looks good. I'm a bit worried about the implications, since there is no way to restore Lock's/etc or any of the recipes based on existing ephemeral nodes. It means that when the program starts up, if it had Lock nodes out there, they'll remain there, even though there's no way for this client to recover the use of them. Does your code using this also save information upon quitting about all the ephemeral nodes and where they were? Best practice with Zookeeper has generally been to avoid attempting to recover a session, and if your process needs to exit, it's better to start over. |
|
Regarding the tests, it looks like this change introduces some issues with the way the connection is handled. While the test you added passes, a lot of others no longer pass, but they passed before this change so there's definitely more work to be done as it appears to have unintended side-effects. |
Our code expects to find ephemeral nodes at specific places. If it's not there, we can know that one client or another lost the session, and thus act on this.
Hm, ok, I'll look at them. Thanks! |
|
Regarding documentation, I think the only addition that would be useful is a disclaimer/warning in the doc string that using keep_session with Lock's or other recipe objects using ephemeral nodes could be problematic. Without any docs warning about this option, it seems quite easy for someone to think they do want the session to stay not understanding how bad it could be. I can't think of any other option with as much potential for issues as this one. |
|
@ralt Did you want to look into fixing the tests so they pass for the others, or did the change introduce something to the connection handling too subtle to pick out easily? |
|
@bbangert I want to look into it, just haven't had the time yet, it's a scarce resource :-) |
| @@ -113,7 +113,7 @@ def __init__(self, hosts='127.0.0.1:2181', | |||
| timeout=10.0, client_id=None, handler=None, | |||
| default_acl=None, auth_data=None, read_only=None, | |||
| randomize_hosts=True, connection_retry=None, | |||
| command_retry=None, logger=None, **kwargs): | |||
| command_retry=None, logger=None, keep_session=False, **kwargs): | |||
There was a problem hiding this comment.
line too long (80 > 79 characters)
|
Updated the base branch to trigger a test run, this should help ensure whether #305 was the issue. |
|
Oh, guess not, tests still all fail. |
|
Just a heads-up, we're coming up on 30 days now. I'm trying to avoid having PR's idle eternally in the backlog like this, so if you'd like to try later, a new PR can be opened when ready. |
|
Yes, let me close that and I'll open a new PR when I'll have some time to look at the tests. |
|
Thanks! |
The use case is being able to restart a process (e.g. software upgrade)
and not losing the ephemeral nodes. The process can save the session id
and password in a file, reload them at boot time, and load the kazoo
client with the previous session id.