[Merged by Bors] - Don't limit get hash requests for fetching ATX deps #5467
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
poszu
force-pushed
the
dont-limit-fetching-atx-deps
branch
from
130480f to
6a9a733
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5467 +/- ##
=======================================
Coverage 77.6% 77.7%
=======================================
Files 266 267 +1
Lines 30889 30901 +12
=======================================
+ Hits 23997 24011 +14
+ Misses 5377 5375 -2
Partials 1515 1515 ☔ View full report in Codecov by Sentry. |
fasmat
approved these changes
Jan 18, 2024
Comment on lines
+29
to
44
| type GetAtxOpts struct { | ||
| LimitingOff bool | ||
| } | ||
|
|
||
| type GetAtxOpt func(*GetAtxOpts) | ||
|
|
||
| func WithoutLimiting() GetAtxOpt { | ||
| return func(opts *GetAtxOpts) { | ||
| opts.LimitingOff = true | ||
| } | ||
| } | ||
|
|
||
| // AtxFetcher defines an interface for fetching ATXs from remote peers. | ||
| type AtxFetcher interface { | ||
| GetAtxs(context.Context, []types.ATXID) error | ||
| GetAtxs(context.Context, []types.ATXID, ...GetAtxOpt) error | ||
| } |
There was a problem hiding this comment.
NIT: I feel like these options and the interface don't belong here; the options should be where the implementation is (fetch package) and the interface probably where it is used (atxsync and activation package?)
|
bors merge |
|
bors cancel |
|
Canceled. |
|
bors merge |
spacemesh-bors bot
pushed a commit
that referenced
this pull request
Jan 18, 2024
## Motivation The original limiting of ATX fetching didn't consider the recursiveness of fetching an ATX (fetching an ATX can trigger fetching its dependencies). The simultaneous fetching of many ATXs could stall, waiting for a slot to fetch dependencies, without a way to unblock and starve. ## Changes Don't limit ATX fetches when fetching dependencies of a processed ATX. Huge batch fetches of ATXs from processing an active set or epoch info are limited. ## Test Plan I ran against the mainnet and didn't observe fetch stalls.
pigmej
approved these changes
Jan 18, 2024
|
Pull request successfully merged into develop. Build succeeded: |
spacemesh-bors
bot
changed the title
poszu
added a commit
that referenced
this pull request
Jan 18, 2024
The original limiting of ATX fetching didn't consider the recursiveness of fetching an ATX (fetching an ATX can trigger fetching its dependencies). The simultaneous fetching of many ATXs could stall, waiting for a slot to fetch dependencies, without a way to unblock and starve. Don't limit ATX fetches when fetching dependencies of a processed ATX. Huge batch fetches of ATXs from processing an active set or epoch info are limited. I ran against the mainnet and didn't observe fetch stalls.
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.
Motivation
The original limiting of ATX fetching didn't consider the recursiveness of fetching an ATX (fetching an ATX can trigger fetching its dependencies). The simultaneous fetching of many ATXs could stall, waiting for a slot to fetch dependencies, without a way to unblock and starve.
Changes
Don't limit ATX fetches when fetching dependencies of a processed ATX. Huge batch fetches of ATXs from processing an active set or epoch info are limited.
Test Plan
I ran against the mainnet and didn't observe fetch stalls.