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

Events notification system for Nessie - VersionStore changes #6647

Merged
merged 26 commits into from May 5, 2023

Conversation

adutra
Copy link
Contributor

@adutra adutra commented Apr 21, 2023

This PR is part of a series of PRs related to the events notification system for Nessie.

See design doc: https://github.com/projectnessie/nessie/blob/main/design/events.md.

This PR focuses on changes required to the existing VersionStore API and implementations.

Summary of changes:

  • New EventsVersionStore implementation following the delegate pattern already used for metrics and tracing. Not used for now.
  • Most methods in VersionStore now return more detailed results:
    • commit() returns CommitResult
    • assign() returns ReferenceAssignedResult
    • create() returns ReferenceCreatedResult
    • delete() returns ReferenceDeletedResult
  • The existing MergeResult now has more methods:
    • BranchName getSourceBranch();
    • List<COMMIT> getAddedCommits(); this is the most tricky addition
  • The new LazyPut interface: contrary to Put, it does not deserialize the content value until it's necessary to do so. (The goal is to avoid deserializing contents on Nessie's write path, if the REST API doesn't need it.)

All these additions are meant to be consumed by the events system, not by the REST API which does not change at all, as required by the design doc.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

Not a full review (and not done yet), but went though a bunch of files.

Adding all the *Result objects to the VersionStore functions is fine.
Not so much a fan of the new "top-leve" LazyPut (commented).

I suspect, with some of the suggestions applied, the size of the PR will reduce a bit.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

Thanks!
I'll take a closer look probably next week.
Just a few rather minor comments for now, nothing serious.

@adutra adutra force-pushed the events-versionstore-standalone branch 4 times, most recently from 941b648 to ba74459 Compare May 3, 2023 13:33
@adutra adutra force-pushed the events-versionstore-standalone branch 3 times, most recently from 03ef247 to 8336765 Compare May 4, 2023 09:33
@adutra adutra force-pushed the events-versionstore-standalone branch from 8336765 to 12300f1 Compare May 5, 2023 08:41
Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

Veeeery close :)

Just a few more minor-ish comments and then we can ship it.

break;
default:
throw new UnsupportedOperationException("Unknown opVariant " + opVariant);
}

if (!casSuccess) {
if (opVariant == CasOpVariant.COMMIT) {
if (opVariant == CasOpVariant.COMMIT || opVariant == CasOpVariant.MERGE) {
cleanUpCommitCas(ctx, individualCommits, individualKeyLists);
Copy link
Member

Choose a reason for hiding this comment

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

Just noting (nothing to change here) - only for posterity why this is not in the new model:
Initially I ported this behavior to the new storage model as well, but (manual, bad "heavy load") tests proved that this can go sideways - up to the point that two concurrent writers produce the exact same commit, one wins and bumps HEAD, the other one "loses" and deletes the commit -> HEAD points to a non-existing commit. It's a rather artificial and will probably never happen in real-live, but still, there's a chance.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

Boom! Ship it! :)

@adutra adutra merged commit b9a923e into projectnessie:main May 5, 2023
24 checks passed
@adutra adutra deleted the events-versionstore-standalone branch May 5, 2023 13:09
@snazy snazy mentioned this pull request May 12, 2023
6 tasks
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.

None yet

2 participants