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

adding ledgerbackend datastore txmeta as a data source #235

Merged
merged 8 commits into from
Apr 22, 2024

Conversation

chowbao
Copy link
Contributor

@chowbao chowbao commented Mar 24, 2024

Integrate cdp changes to ingest from datastore txmeta files

  • Added new option to read/create ledgerbackend using the ledgerbackend.datastore to interface with the txmeta files from gcs
  • Controlled by two new parameters
    • captive-core - will use captive core; datastore gcs is the default for stellar-etl now
    • datastore-url - the url to the datastore (gcs bucket)
  • Most export_* could use the LedgerCloseMeta to transform/process the data needed
  • assets and ledgers which originally pulled from history archives had a custom xdr format that needed to be manually constructed from the LedgerCloseMeta
    • TODO: In the future could consider removing this dependency all together and use straight LedgerCloseMeta instead (different PR)
  • TODO: Probably should delete all the unused from_genesis export_*s (different PR)
  • TODO: All the parameters are kinda a mess; could be refactored (different PR)
  • TODO: The export_* commands could also be refactored (different PR)

Copy link
Contributor

@sydneynotthecity sydneynotthecity left a comment

Choose a reason for hiding this comment

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

Great start for ETL refactor. Small nit to get the unit test passing for operation_trace_code again.

I agree with all of your TODOs listed and think we should document them in tickets to be working in an upcoming sprint.

Did you test the code using the captive_core flag to confirm that the code is backwards compatible?

cmd/export_ledger_entry_changes.go Show resolved Hide resolved
internal/input/assets_history_archive.go Outdated Show resolved Hide resolved
internal/input/ledgers_history_archive.go Outdated Show resolved Hide resolved
internal/input/operations.go Show resolved Hide resolved
internal/utils/main.go Outdated Show resolved Hide resolved
Comment on lines 31 to 33
paymentOps, err = input.GetPaymentOperationsHistoryArchive(startNum, endNum, limit, isTest, isFuture)
} else {
paymentOps, err = input.GetPaymentOperations(startNum, endNum, limit, env, useCaptiveCore)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like your idea of eventually refactoring this History Archives method to use lcm directly. I think it's cleaner and less confusing

@sydneynotthecity
Copy link
Contributor

Did you test the code using the captive_core flag to confirm that the code is backwards compatible?

I see that the test CDP DAGs only use one image_name so yes it is backwards compatible

@chowbao chowbao marked this pull request as ready for review April 19, 2024 15:03
@chowbao chowbao requested a review from a team as a code owner April 19, 2024 15:03
@chowbao
Copy link
Contributor Author

chowbao commented Apr 19, 2024

Before merging:


// AccountSignersChanged returns true if account signers have changed.
// Notice: this will return true on master key changes too!
func AccountSignersChanged(c ingest.Change) bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was removed from the ingest library. Needed for account signers

Copy link
Contributor

@sydneynotthecity sydneynotthecity left a comment

Choose a reason for hiding this comment

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

LGTM

@chowbao chowbao merged commit a334cb5 into master Apr 22, 2024
4 checks passed
@sydneynotthecity sydneynotthecity deleted the cdp-integration branch July 15, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants