Credit first peer to return content #1447
Merged
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
carver
force-pushed
the
natural-connection-id
branch
from
892766f
to
f2a4267
Compare
carver
changed the title
carver
force-pushed
the
natural-connection-id
branch
2 times, most recently
from
33e3129
to
bad2ca3
Compare
carver
changed the title
carver
force-pushed
the
natural-connection-id
branch
from
279d788
to
bad2ca3
Compare
19 tasks
morph-dev
approved these changes
Sep 13, 2024
| break; | ||
| } | ||
| // Split off the next up to `parallelism` expected peers. | ||
| else if expected.len() < max_parallelism { | ||
| else if remaining.len() < max_parallelism { |
There was a problem hiding this comment.
nit: can't we simplify this to:
expected = remaining.drain(..min(max_parallelism, remaining.len()));
njgheorghita
approved these changes
Sep 13, 2024
Find Content tests were challenging to debug. Add more logged info when the test fails, and showing important milestones of the test cycle. Additionally, refactor the naming of `remaining` and `expected` a bit to make reading the test more welcoming to newcomers.
When multiple peers return the content, the result should give credit to the first peer that responds, not the last. Also, this bug was not found locally because 10 iterations wasn't enough to expose the issue the first time. Increasing to 100, which still takes only 200ms, and was reliably reproducing this issue.
In ethportal-peertest, it showed that a file was missing, but not which one. That information is critical, and self_peertest runs slowly, so reducing reruns is very valuable.
When checking for a local copy of content requested by a peer, the comment explaining the case for missing content should match the order of the pattern. Also, the pattern itself is made a little clearer.
carver
force-pushed
the
natural-connection-id
branch
from
279d788
to
65ef35e
Compare
1 task
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.
What was wrong?
While working on #1395, a number of cleanups accumulated. But there's enough going on in that PR that it's nice to separate out the stuff that can be. This is a mashup of those changes, with more info in each commit.
Most prominent, and the only change in functionality, is that when content arrives twice, we ignore the 2nd to arrive (instead of ignoring the first to arrive). We would like to give credit to the fastest peer to return the content.
How was it fixed?
Just ignore additional content to come in. This of course continues to mean that if the first content to come in is invalid, then the whole query fails. #1395 is the next step toward improving that behavior.
Everything else is just:
To-Do