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.

Sign up for GitHub

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

Jump to bottom

New Feature: Adds the sync mode importer #1415

Merged
merged 12 commits into from
Jan 25, 2023
Merged

New Feature: Adds the sync mode importer #1415

merged 12 commits into from
Jan 25, 2023

Conversation

AlgoStephenAkiki
Copy link
Contributor

@AlgoStephenAkiki AlgoStephenAkiki commented Jan 13, 2023 •
edited
Loading

Keeps all interfaces the same but adds specific request functionality to the algod importer.

@AlgoStephenAkiki AlgoStephenAkiki changed the base branch from develop to feature/sync-mode January 13, 2023 20:03
Eric-Warehime
Eric-Warehime suggested changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@Eric-Warehime Eric-Warehime left a comment

Choose a reason for hiding this comment

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

Can you add a description to the PR also?

helpers/helpers.go Outdated Show resolved Hide resolved
shiqizng
shiqizng reviewed Jan 13, 2023
View reviewed changes
conduit/plugins/importers/algod/algod_importer.go Outdated
@@ -32,6 +32,11 @@ type algodImporter struct {
cancel context.CancelFunc
}

func (algodImp *algodImporter) OnComplete(input data.BlockData) error {
_, err := algodImp.aclient.SetSyncRound(input.Round() + 1).Do(algodImp.ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

what if input.Round() is < current round in ledger? I think you would also need to call SetSyncRound in Init?
and does algod node need to have a large enough MaxAcctLookback for indexer to import older rounds?

Copy link
Contributor

Choose a reason for hiding this comment

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

The node will be required to have the delta in the cache. This means the indexer's round must be within MaxAcctLookback of the node's round. The data node will set the sync round to the latest round on startup to make sure it doesn't get ahead of the indexer.

@shiqizng
Copy link
Contributor

can we rebase this PR to develop instead of feature/sync-mode?

@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 1321-replace-block-processor-with-sync-mode-api-importer branch from 0aabd10 to 38fcec9 Compare January 17, 2023 20:07
@codecov
Copy link

codecov bot commented Jan 17, 2023 •
edited
Loading

Codecov Report

Merging #1415 (c72038c) into develop (63bdb86) will increase coverage by 0.06%.
The diff coverage is 73.19%.

@@             Coverage Diff             @@
##           develop    #1415      +/-   ##
===========================================
+ Coverage    65.05%   65.12%   +0.06%     
===========================================
  Files           79       80       +1     
  Lines        11276    11373      +97     
===========================================
+ Hits          7336     7407      +71     
- Misses        3374     3394      +20     
- Partials       566      572       +6
Impacted Files Coverage Δ
.../importers/algodfollower/algodfollower_importer.go 73.19% <73.19%> (ø)
fetcher/fetcher.go 62.71% <0.00%> (-1.28%) ⬇️
idb/postgres/postgres.go 65.35% <0.00%> (+0.13%) ⬆️

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

@AlgoStephenAkiki AlgoStephenAkiki marked this pull request as ready for review January 18, 2023 03:16
@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 1321-replace-block-processor-with-sync-mode-api-importer branch 2 times, most recently from aad98ac to 8989f61 Compare January 18, 2023 18:05
@AlgoStephenAkiki AlgoStephenAkiki changed the base branch from feature/sync-mode to develop January 18, 2023 18:05
@AlgoStephenAkiki AlgoStephenAkiki changed the title Updates New Feature: Adds the sync mode importer Jan 18, 2023
@AlgoStephenAkiki AlgoStephenAkiki self-assigned this Jan 18, 2023
@AlgoStephenAkiki
Copy link
Contributor Author

can we rebase this PR to develop instead of feature/sync-mode?

I have rebased this against develop

Eric-Warehime
Eric-Warehime reviewed Jan 18, 2023
View reviewed changes
idb/postgres/postgres_integration_test.go Outdated Show resolved Hide resolved
helpers/helpers.go Outdated Show resolved Hide resolved
go.mod Outdated
@@ -8,6 +8,7 @@ require (
github.com/algorand/avm-abi v0.2.0
github.com/algorand/go-algorand v0.0.0-20220211161928-53b157beb10f
github.com/algorand/go-algorand-sdk v1.22.1-0.20221209152459-223656f08456
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need v1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we do but I'd rather save that for another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, sounds fine.

conduit/plugins/importers/syncmode/syncmode_importer_config.go Outdated Show resolved Hide resolved
conduit/plugins/importers/syncmode/syncmode_importer.go Outdated Show resolved Hide resolved
@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 1321-replace-block-processor-with-sync-mode-api-importer branch from 030889d to e6a184a Compare January 19, 2023 01:06
@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 1321-replace-block-processor-with-sync-mode-api-importer branch from 9cafdea to 380e708 Compare January 19, 2023 02:36
@winder winder mentioned this pull request Jan 19, 2023
Merged
@AlgoStephenAkiki
Updates
08e1721 
Removed helper funcs

Changed to syncmode importer

Fmt and lint

Reverted algod importer
@AlgoStephenAkiki
Rebased against develop
2b36450
@AlgoStephenAkiki
linting
853d93c
@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 1321-replace-block-processor-with-sync-mode-api-importer branch from ae09ea0 to 487307f Compare January 23, 2023 18:41
@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 1321-replace-block-processor-with-sync-mode-api-importer branch from e567fe7 to f6e9bf0 Compare January 23, 2023 18:58
@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 1321-replace-block-processor-with-sync-mode-api-importer branch from 3dbea36 to c72038c Compare January 24, 2023 20:29
@winder winder merged commit 451d4b3 into develop Jan 25, 2023
@winder winder deleted the 1321-replace-block-processor-with-sync-mode-api-importer branch January 25, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winder winder winder left review comments

@shiqizng shiqizng shiqizng approved these changes

@Eric-Warehime Eric-Warehime Eric-Warehime approved these changes

Assignees

@AlgoStephenAkiki AlgoStephenAkiki

Labels
New Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlgoStephenAkiki @shiqizng @winder @Eric-Warehime