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

Use the latest version of the Stellar ingestion library #122

Merged
merged 5 commits into from
Feb 19, 2021

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Feb 16, 2021

I have updated the stellar-etl repo to use the non experimental ingestion library which we have just published:

https://github.com/stellar/go/releases/tag/stellar-ingest-v1.0.0

The API for the ingestion library should be somewhat stable now. So do not expect many breaking changes in the future.

@@ -2,9 +2,9 @@ package input

import (
"fmt"
"github.com/stellar/go/historyarchive"
Copy link

Choose a reason for hiding this comment

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

Move to next group.

Comment on lines 4 to 7
"github.com/stellar/go/ingest"
"github.com/stellar/go/xdr"
"github.com/stellar/stellar-etl/internal/utils"
"io"
Copy link

Choose a reason for hiding this comment

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

Suggested change
"github.com/stellar/go/ingest"
"github.com/stellar/go/xdr"
"github.com/stellar/stellar-etl/internal/utils"
"io"
"io"
"github.com/stellar/go/ingest"
"github.com/stellar/go/xdr"
"github.com/stellar/stellar-etl/internal/utils"

internal/transform/offer.go Outdated Show resolved Hide resolved
internal/transform/operation.go Outdated Show resolved Hide resolved
internal/transform/trade.go Outdated Show resolved Hide resolved
internal/transform/transaction.go Outdated Show resolved Hide resolved
Comment on lines 7 to 8
"github.com/stellar/go/historyarchive"
"github.com/stellar/go/ingest/ledgerbackend"
Copy link

Choose a reason for hiding this comment

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

Move out of std group.

return historyArchiveBackend{client: client, ledgers: ledgers}, nil
}

func CreateHistoryArchiveClient() (historyarchive.ArchiveInterface, error) {
archiveStellarURL := "http://history.stellar.org/prd/core-live/core_live_001"
Copy link

Choose a reason for hiding this comment

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

Not in a scope of this PR but this should be https because tx data is not signed.

Suggested change
archiveStellarURL := "http://history.stellar.org/prd/core-live/core_live_001"
archiveStellarURL := "https://history.stellar.org/prd/core-live/core_live_001"

Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise not in scope, but using the new historyarchive.NewArchivePool (stellar/go#3402) with multiple URLs here could be useful here for resilience.

}

err = validateLedgerRange(start, end, latestNum)
backend, err := utils.CreateBackend(start, end)

Choose a reason for hiding this comment

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

does a lot of the validation happen automatically now? @tamirms

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 noticed that a lot of the checks where repeated every time a backend was created so I thought it would be better to bundle the checks in utils.CreateBackend() to avoid repeated code

Copy link

@nikhilsaraf nikhilsaraf left a comment

Choose a reason for hiding this comment

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

Thank you for making this change!

From what I understand, this is mostly a refactor of patterns into the utility function and not much in the API usage for history archives has changed other than using the interface rather than the concrete backend object in these structs. Is that right?

Am I missing anything? It's possible that I missed the main area that contained the functional change.

Also, were you able to test it by running one of the commands? If so, did that give expected results?

Copy link

@nikhilsaraf nikhilsaraf left a comment

Choose a reason for hiding this comment

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

If the comment I made above is true, that there wasn't much of a change with the API and we've run the tool and gotten expected results then we can go ahead and merge. I can try and deploy this early in the coming week while we all still have context

@tamirms
Copy link
Contributor Author

tamirms commented Feb 18, 2021

From what I understand, this is mostly a refactor of patterns into the utility function and not much in the API usage for history archives has changed other than using the interface rather than the concrete backend object in these structs. Is that right?
Am I missing anything? It's possible that I missed the main area that contained the functional change.

That's right, we've named some package names and did some structural refactoring. Otherwise, the functionality should be identical to what was provided before.

Also, were you able to test it by running one of the commands? If so, did that give expected results?

I have not but I will try to see if it's possible to run the commands locally

@nikhilsaraf
Copy link

Great, if we can run locally to test it would be great. Otherwise good to merge on my end 🎉

@tamirms
Copy link
Contributor Author

tamirms commented Feb 19, 2021

I have run the History Archive Commands on some sample inputs and compared the output with running the same commands using the stellar-etl code in the master branch. The outputs match! So, I will merge the PR

@tamirms tamirms merged commit 832d80b into master Feb 19, 2021
@tamirms tamirms deleted the ingestion-library branch February 19, 2021 14:49
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.

4 participants