-
Notifications
You must be signed in to change notification settings - Fork 452
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
FateId to use UUID instead of long #4388
Conversation
- 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
There was a problem hiding this 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
.../src/main/java/org/apache/accumulo/manager/compaction/coordinator/CompactionCoordinator.java
Show resolved
Hide resolved
There was a problem hiding this 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()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
I am going to open a follow on PR w/ the unit test changes I suggested |
closes #4277
Changes:
Note: