Skip to content

catchpoints: check EnableCatchupFromArchiveServers for ledgerFetcher#5783

Merged
gmalouf merged 2 commits intoalgorand:masterfrom
ohill:catchpoint-archiver-peer-selector
Oct 13, 2023
Merged

catchpoints: check EnableCatchupFromArchiveServers for ledgerFetcher#5783
gmalouf merged 2 commits intoalgorand:masterfrom
ohill:catchpoint-archiver-peer-selector

Conversation

@ohill
Copy link
Copy Markdown
Contributor

@ohill ohill commented Oct 13, 2023

Summary

The ledgerFetcher is used by the catchpointService to download the catchpoint file (ledger snapshot) and uses a peerSelector that only connects to relays. This change updates it to use the same makePeerSelector arguments as used when downloading blocks as part of fast catchup, where EnableCatchupFromArchiveServers is checked.

Fixes #5765.

Test Plan

Existing tests should pass.

@ohill ohill requested a review from winder October 13, 2023 15:07
@ohill ohill added the Bug-Fix label Oct 13, 2023
@ohill ohill requested a review from gmalouf October 13, 2023 15:15
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 13, 2023

Codecov Report

Merging #5783 (ebbe007) into master (0358d1d) will increase coverage by 0.00%.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##           master    #5783   +/-   ##
=======================================
  Coverage   55.46%   55.47%           
=======================================
  Files         473      473           
  Lines       66720    66722    +2     
=======================================
+ Hits        37005    37012    +7     
+ Misses      27197    27189    -8     
- Partials     2518     2521    +3     
Files Coverage Δ
catchup/catchpointService.go 8.44% <50.00%> (+0.40%) ⬆️

... and 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Copy Markdown
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look like it would hurt anything, but I don't know the catchpoint download retry logic well enough to tell if it would solve #5765.

Additional context: #5393

@gmalouf gmalouf merged commit d3df476 into algorand:master Oct 13, 2023
@ohill ohill deleted the catchpoint-archiver-peer-selector branch October 16, 2023 13:20
@ohill
Copy link
Copy Markdown
Contributor Author

ohill commented Oct 16, 2023

This doesn't look like it would hurt anything, but I don't know the catchpoint download retry logic well enough to tell if it would solve #5765.

Additional context: #5393

There are two peerSelectors updated in this PR. The first one is created in processStageLedgerDownload to download the catchpoint file. The second one is created in checkLedgerDownload which was added in #5393. Now they both use the same peerSelector as used to download blocks during fast catchup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EnableCatchupFromArchiveServers option does not work

4 participants