Optimize unary handlers with persistent connections to P2P daemon #328
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #328 +/- ##
==========================================
+ Coverage 83.22% 83.40% +0.17%
==========================================
Files 66 66
Lines 6002 6139 +137
==========================================
+ Hits 4995 5120 +125
- Misses 1007 1019 +12
|
borzunov
changed the title
borzunov
changed the title
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Jul 19, 2021
borzunov
reviewed
Aug 20, 2021
borzunov
reviewed
Aug 20, 2021
justheuristic
requested changes
Aug 20, 2021
There was a problem hiding this comment.
- in DaemonConnector create_persistent_connection, await an ok message from daemon to ensure that it was created successfully before returning connection
- add a test that attempts to call: (1) non-existing unary handler (2) existing handler with wrong protobuf types (3) self-dial [non-]existing handler. Ensure that it raises error instead of hanging
- p2pd: do not send ok (p2pd->pyclient) on receiving unary response
- update p2pd versions (merge libp2p master into ours) - libp2p/go-libp2p-daemon@193eab6
- print warning on any unexpected messages over persistent connection
hivemind/p2p/servicer.py
Outdated
| @@ -131,6 +131,7 @@ async def add_p2p_handlers(self, p2p: P2P, wrapper: Any = None, *, namespace: Op | |||
| getattr(servicer, handler.method_name), | |||
| handler.request_type, | |||
| stream_input=handler.stream_input, | |||
There was a problem hiding this comment.
Nitpicking: should we create them asynchronously via gather
There was a problem hiding this comment.
upd: this is definitely not a bottleneck, feel free to ignore/resolve
There was a problem hiding this comment.
I support this because it may improve startup time. Good catch!
|
Just in case, the |
borzunov
reviewed
Aug 23, 2021
Co-authored-by: Alexander Borzunov <[email protected]>
… into unary-handlers
borzunov
approved these changes
Aug 24, 2021
There was a problem hiding this comment.
Approving this. We have also decided to revert the first change from learning-at-home/go-libp2p-daemon#11 in a future PR (it is unnecessary but increases startup time).
justheuristic
approved these changes
Aug 24, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements unary handlers over a persistent daemon connection.
The daemon part (Golang): learning-at-home/go-libp2p-daemon#3