Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

algod importer: Combines algod and algod_follower #1452

Merged
merged 9 commits into from
Feb 16, 2023

Conversation

AlgoStephenAkiki
Copy link
Contributor

Resolves #1449

Combines the algod and algod_follower plugins and uses a config value to switch between the logic of them

@AlgoStephenAkiki AlgoStephenAkiki self-assigned this Jan 31, 2023
@AlgoStephenAkiki AlgoStephenAkiki linked an issue Jan 31, 2023 that may be closed by this pull request
@AlgoStephenAkiki AlgoStephenAkiki marked this pull request as ready for review January 31, 2023 22:04
@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 1449-combine-algod-based-plugin-logic branch from ab07699 to 2daa8a7 Compare February 1, 2023 15:43
@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #1452 (7aca164) into develop (cb4581c) will decrease coverage by 0.13%.
The diff coverage is 68.29%.

@@             Coverage Diff             @@
##           develop    #1452      +/-   ##
===========================================
- Coverage    65.28%   65.16%   -0.13%     
===========================================
  Files           83       81       -2     
  Lines        11452    11369      -83     
===========================================
- Hits          7477     7409      -68     
+ Misses        3404     3395       -9     
+ Partials       571      565       -6     
Impacted Files Coverage Δ
conduit/plugins/importers/algod/algod_importer.go 77.96% <68.29%> (-4.65%) ⬇️
fetcher/fetcher.go 63.98% <0.00%> (+1.27%) ⬆️

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

Resolves #1449

Combines the algod and algod_follower plugins and uses a config value to
switch between the logic of them
@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 1449-combine-algod-based-plugin-logic branch from 2daa8a7 to 01b06eb Compare February 1, 2023 18:18
Copy link
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.

Two main changes:

  1. Find a way to avoid a string comparison to toggle functionality in the GetBlock path.
  2. Fix the tests. It looks like you applied the same pattern blindly to all tests. Most of them don't seem to add any coverage, the ones that do don't have any new assertions to check that the functionality is correct.

@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 1449-combine-algod-based-plugin-logic branch from 13900c2 to a26e476 Compare February 2, 2023 20:04
@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 1449-combine-algod-based-plugin-logic branch from a26e476 to b46cb57 Compare February 2, 2023 20:07
@winder winder changed the title New Feature: Combines algod and algod_follower algod importer: Combines algod and algod_follower Feb 3, 2023
@Eric-Warehime Eric-Warehime requested review from shiqizng and removed request for mciccarello February 15, 2023 15:16
@@ -155,23 +155,18 @@ func (algodImp *algodImporter) GetBlock(rnd uint64) (data.BlockData, error) {
var blk data.BlockData

for retries := 0; retries < 3; retries++ {
// nextRound - 1 because the endpoint waits until the subsequent block is committed to return
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this retry loop is necessary. there's already retries at the pipeline level. in this loop there's no wait before the next try, could this loop always finish before the next block is available?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, just added retires w/ backoff to the most recent version.

for r := 0; r < retries; r++ {
		time.Sleep(time.Duration(waitMultiplier*float64(r)) * initialWait)
...

We could just rely on the pipeline-level retires like you said, but retries with backoff make more sense in the case of the follower node since the catchup service gets started/stopped so frequently.

@winder winder added Enhancement New feature or request and removed New Feature labels Feb 16, 2023
@winder winder merged commit d37f828 into develop Feb 16, 2023
@winder winder deleted the 1449-combine-algod-based-plugin-logic branch February 16, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combine algod-based plugin logic
4 participants