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

FateId to use UUID instead of long #4388

Merged
merged 4 commits into from
Mar 16, 2024

Conversation

kevinrr888
Copy link
Member

closes #4277
Changes:

  • FateId now uses a 128bit UUID instead of a (64bit) long
    • FateIds created by ZooStore and AccumuloStore now use a v4 UUID (random UUID)
    • FateIds created by FateIdGenerator now use a v3 UUID (name based) so the same FateKey gives the same UUID
  • Necessary updates to classes and tests which used the long id

Note:

  • Unfortunately, there is no simple way to include the creation time in the UUID of a FateId since FateIdGenerator requires that the same FateKey generates the same FateId. So, at least for now, FateId's UUID does not include a timestamp.

- FateId now uses a 128bit UUID instead of a (64bit) long
	- FateIds created by ZooStore and AccumuloStore now use a v4 UUID (random UUID)
	- FateIds created by FateIdGenerator now use a v3 UUID (name based) so the same FateKey gives the same UUID
- Necessary updates to classes and tests which used the long id
Copy link
Contributor

@keith-turner keith-turner left a comment

Choose a reason for hiding this comment

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

Still looking at this

Copy link
Contributor

@keith-turner keith-turner left a comment

Choose a reason for hiding this comment

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

These changes look good, could not really review all the variable name changes which is why I asked if they were done w/ an ide. Would be good to add the unit test for illegal fate ids.

@@ -1086,15 +1083,15 @@ public void testCompactedAndFlushIdFilter() {
ConditionalTabletsMutatorImpl ctmi = new ConditionalTabletsMutatorImpl(context);
Set<TabletMetadataFilter> filter = Set.of(new TestTabletMetadataFilter());
FateInstanceType type = FateInstanceType.fromTableId(tid);
FateId fateId34L = FateId.from(type, 34L);
FateId fateId987L = FateId.from(type, 987L);
FateId fateId1 = FateId.from(type, UUID.randomUUID());
Copy link
Contributor

Choose a reason for hiding this comment

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

Were these test variable renames done using an ide?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and I also double checked that the variables remained consistent

@keith-turner keith-turner merged commit eb71e3e into apache:elasticity Mar 16, 2024
8 checks passed
@keith-turner
Copy link
Contributor

I am going to open a follow on PR w/ the unit test changes I suggested

keith-turner added a commit to keith-turner/accumulo that referenced this pull request Mar 16, 2024
@keith-turner keith-turner linked an issue Mar 16, 2024 that may be closed by this pull request
@DomGarguilo DomGarguilo added this to the 4.0.0 milestone Jul 12, 2024
@kevinrr888 kevinrr888 deleted the elasticity-feature-4277 branch November 1, 2024 15:06
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.

FateId's tid should be replaced with 128bit tid
3 participants