sdk/python: dial routers and DHT peers at once, stop when the record is in hand - #536
Merged
Merged
Conversation
…is in hand The bananas deploy of bd58efb failed the SDK cold-path probe again: the Python call took over 120s on its first attempt (probe.sh now says so: "hung for 120s"). Timed in the probe's pod, the cold path was enroll 0.2s, join 16s, discover 15s, connect 0.2s: each 15s is one bounded dial to the router the control plane lists that the cluster's network policy drops, and every step ran one dial after another. Right after a rollout the routers' tables also name the pods it just replaced as closer peers, each another 15s in the walk, which is what pushed the first attempt past the cap on the deploy and made it a coin toss on the hub. - The DHT walk both lookups share (`_walk`) asks a round's peers at once and returns as soon as an answer satisfies the lookup; only otherwise are the closer peers dialed, at once too. A round costs one query and one dial timeout, not one per peer. The duplicate GET_PROVIDERS query helper goes. - `_admit` dials and authenticates every router at once; the reservation still goes to the first admitted router of the list, so a peer is reachable through the same router as before. - Tests: providers in hand end the walk with no dial; three stale closer peers cost one dial timeout together; a slow router does not delay the others' answers; join with a dark router first in the list ends at DIAL_TIMEOUT with the reservation on the first router that admitted. Measured in the same pod with this change, three cold runs each: join 15.3s (the dark router's timeout, alone), discover 0.1s, connect 0.3s, call 0.2s.
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the Kademlia walk in discovery and the router admission process in session management to perform network queries and dials concurrently using Trio nurseries, significantly reducing the impact of slow or unreachable peers. New tests have been added to verify these concurrent behaviors. The feedback suggests refactoring a list comprehension inside an any() call to prevent potential short-circuiting bugs if it is ever refactored into a generator expression, ensuring all provider answers are always accumulated.
A generator expression there would stop at the first router's providers; the loop reads as what it is.
This file contains hidden or 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
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.
The bananas deploy of bd58efb failed the SDK cold-path probe again (https://github.com/google/sam/actions/runs/36338262423/job/108677106672); with #532's
timeoutthe probe now reports it instead of hanging:Measured in the probe's pod (same image, token and env)
Cold path before: enroll 0.2s, join 16s, discover 15s, connect 0.2s. Each 15s is one bounded dial to the GCE router the control plane lists but the cluster's network policy drops — and every step dialed one peer after another. Right after a rollout the routers' tables also name the pods just replaced as closer peers (another 15s each), which is what pushed the first attempt past 120s on the deploy and made the hub's first attempt a coin toss.
After, three cold runs: join 15.3s (the dark router's timeout, alone), discover 0.1s, connect 0.3s, call 0.2s.
Changes
_walk) forfind_peer/find_providers: a round asks its peers at once and returns as soon as an answer satisfies the lookup; only otherwise are the closer peers dialed, also at once. A round costs one query and one dial timeout, not one per peer. Drops the duplicate GET_PROVIDERS helper._admitdials and authenticates every router at once; the reservation still goes to the first admitted router of the list (same reachability semantics as before).DIAL_TIMEOUTwith the reservation on the first admitting router.Validation
sdk/python: 91 passedgo test ./tests/integration -run 'TestNativeSDK|TestSDK': all pass (incl.TestNativeSDKsAcrossRouters, JS and Python)