Add sync multiprocess support #3972
Conversation
liorsve
force-pushed
the
sync-multiprocess-support
branch
5 times, most recently
from
4a810ec to
fd00e61
Compare
liorsve
force-pushed
the
python-sync-client
branch
3 times, most recently
from
122d41d to
dda9443
Compare
|
|
||
| return self | ||
|
|
||
| def _reset_client_connection(self): |
There was a problem hiding this comment.
_reset_client_connection -> _reset_client
connection is misleading here
please add documentation
There was a problem hiding this comment.
deleted _reset_client_connection
| conn_req = config._create_a_protobuf_conn_request( | ||
| cluster_mode=type(config) is GlideClusterClientConfiguration | ||
|
|
||
| os.register_at_fork(after_in_child=self._reset_client_connection) |
There was a problem hiding this comment.
so why do we need both _create_new_core_client and _reset_client_connection? can't we just register _create_new_core_client?
There was a problem hiding this comment.
No reason at all, just a leftover from previous iterations where I had different code there and I didn't notice it has no use now
deleted _reset_client_connection
| def _reset_client_connection(self): | ||
| self._create_new_core_client() | ||
|
|
||
| def _create_new_core_client(self): |
There was a problem hiding this comment.
nit: _create_core_client
| except OSError as e: | ||
| pytest.fail(f"Fork failed: {e}") | ||
|
|
||
| if pid == 0: |
There was a problem hiding this comment.
use get_pid() before you fork and compare it to it, it would be more robust
There was a problem hiding this comment.
hope I fixed as you meant
liorsve
force-pushed
the
sync-multiprocess-support
branch
from
fd00e61 to
a9f977d
Compare
Signed-off-by: Lior Sventitzky <[email protected]>
Signed-off-by: Lior Sventitzky <[email protected]>
liorsve
force-pushed
the
sync-multiprocess-support
branch
from
a9f977d to
55dab5b
Compare
Issue link
This Pull Request is linked to issue : #239
Checklist
Before submitting the PR make sure the following are checked: