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

Feature: S3 connection method #464

Merged
merged 7 commits into from
May 9, 2023
Merged

Feature: S3 connection method #464

merged 7 commits into from
May 9, 2023

Conversation

cap10morgan
Copy link
Contributor

Closes #450

Commit 7efcd2f is the heart of this PR.

No tests yet b/c testing S3 is a little tricky. If folks feel strongly there should be some before merging I can figure out a good way to test this locally w/o actually connecting to S3.

It works in my REPL, though! ;)

@cap10morgan cap10morgan requested a review from a team May 5, 2023 20:42
@cap10morgan cap10morgan marked this pull request as draft May 5, 2023 20:52
@cap10morgan
Copy link
Contributor Author

Converted to draft b/c I remembered there's one thing I still need to finish in here. Will ping when it's ready for full review.

Instead of just taking the first ledger that loads successfully, give a target t to aim for and keep loading until we get one that is at that t value or higher.
@cap10morgan
Copy link
Contributor Author

The last commit (971dab8) is unrelated and I'm happy to split it out into its own PR if desired. But I ran into a common test failure here in one of our load tests where the loaded ledger's t is lower than the one we were building up in memory. I added a new test-utils fn that tries to keep loading until we get a ledger w/ a db t of at least that value. So hopefully that will make those tests more reliable.

@cap10morgan cap10morgan marked this pull request as ready for review May 8, 2023 16:21
@cap10morgan
Copy link
Contributor Author

OK @fluree/core this is ready for review

Copy link
Contributor

@zonotope zonotope left a comment

Choose a reason for hiding this comment

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

🪣

@cap10morgan cap10morgan merged commit fe486cd into main May 9, 2023
@cap10morgan cap10morgan deleted the feature/s3-conn branch May 9, 2023 12:28
@bplatz
Copy link
Contributor

bplatz commented May 9, 2023

failure here in one of our load tests where the loaded ledger's t is lower than the one we were building up in memory

If this is after a fluree/commit! which is deref'd then this is a bug that should be high priority and get a new ticket. There should never be a return from commit! until persistence is guaranteed. Perhaps this is specific to a testing tool... not sure, but please advise.

@cap10morgan
Copy link
Contributor Author

failure here in one of our load tests where the loaded ledger's t is lower than the one we were building up in memory

If this is after a fluree/commit! which is deref'd then this is a bug that should be high priority and get a new ticket. There should never be a return from commit! until persistence is guaranteed. Perhaps this is specific to a testing tool... not sure, but please advise.

Just to make sure I'm understanding things correctly:

This is a (deref'd) commit! followed by a load. If I understand things correctly, the load will use the latest pushed head commit when it is called, but push is async. So in these quick commit! then load scenarios in tests, it's possible to load before the push is complete. Does that seem right / expected?

@bplatz
Copy link
Contributor

bplatz commented May 9, 2023

That's OK then... as the name service is intentionally async. We discussed creating an option for name services that could allow ones that can more easily support it to be synchronous and block... but e.g. IPNS as a name service really needs to be async.

However, the return from commit! should be a new db address... in this case there is no need to use the name service, as the only thing it does is tell us the db address that we should already know with the returned db.

I'd suggest any tests with this sort of dependency opt to not use the name service unless there is something specific about the name service itself they are testing.

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.

Implement S3 connection
3 participants