Skip to content

FateId to use UUID instead of long#4388

Merged
keith-turner merged 4 commits into
apache:elasticityfrom
kevinrr888:elasticity-feature-4277
Mar 16, 2024
Merged

FateId to use UUID instead of long#4388
keith-turner merged 4 commits into
apache:elasticityfrom
kevinrr888:elasticity-feature-4277

Conversation

@kevinrr888
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread core/src/main/java/org/apache/accumulo/core/fate/FateId.java
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
Copy Markdown
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
Copy Markdown
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
Copy link
Copy Markdown
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