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.

Sign up for GitHub

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

Jump to bottom

Globally Unique FATE Transaction Ids - Part 3 #4247

Merged
merged 4 commits into from
Feb 14, 2024
Merged

Globally Unique FATE Transaction Ids - Part 3 #4247

merged 4 commits into from
Feb 14, 2024

Conversation

kevinrr888
Copy link
Member

This addresses several previously deferred changes for issue #4044. Root of most of these new changes were from changing TabletMetadata and TabletUpdates (in Ample) - FateId is now stored in the Metadata table instead of just the formatted long id. Addresses deferred changes to TabletMetadata, TabletUpdates, CompactionConfigStorage, SelectedFiles, and Ample.

@kevinrr888
Globally Unique FATE Transaction Ids - Part 3
1f3960d 
This addresses several previously deferred changes for issue apache#4044. Root of most of these new changes were from changing TabletMetadata and TabletUpdates (in Ample) - FateId is now stored in the Metadata table instead of just the formatted long id. Addresses deferred changes to TabletMetadata, TabletUpdates, CompactionConfigStorage, SelectedFiles, and Ample.
@kevinrr888 kevinrr888 marked this pull request as ready for review February 8, 2024 21:17
@kevinrr888 kevinrr888 mentioned this pull request Feb 8, 2024
Closed
@kevinrr888 kevinrr888 mentioned this pull request Feb 12, 2024
Merged
keith-turner
keith-turner reviewed Feb 13, 2024
View reviewed changes
core/src/main/java/org/apache/accumulo/core/metadata/schema/CompactionMetadata.java
@@ -98,7 +99,7 @@ private static class GSonData {
String groupId;
short priority;
boolean propDels;
Long fateTxId;
Copy link
Contributor

Choose a reason for hiding this comment

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

We may need a follow on update for upgrade

@kevinrr888
Review changes:
b70f932 
- FateId refactored (better Pattern for FateId and simpler validation)
- CompactionMetadata GSonData changed to use the canonical FateId string instead of FateId (note that old data may need to be considered here)
- SelectedFiles renamed "txid" -> "fateId". SelectedFilesTest updated to be consistent with changes
- CompactionConfigStorage added Preconditions check
- Changed compactionHints in TabletManagementParameters from Map<FateId,Map<String,String>> -> Map<String,Map<String,String>>: required changes to TabletManagementParameters, CompactionConfigStorage, CompactionJobGenerator, TabletManagementParametersTest, Manager, CancelCompactions, and PreDeleteTable
- AmpleConditionalWriterIT.testCompacted() reordering of FateIds
@kevinrr888
Copy link
Member Author

Completed review changes:

  • FateId refactored (better Pattern for FateId and simpler validation)
  • CompactionMetadata GSonData changed to use the canonical FateId string instead of FateId (note that old data may need to be considered here)
  • SelectedFiles renamed "txid" --> "fateId". SelectedFilesTest updated to be consistent with changes
  • CompactionConfigStorage added Preconditions check
  • Changed compactionHints in TabletManagementParameters from Map<FateId,Map<String,String>> --> Map<String,Map<String,String>>: required changes to TabletManagementParameters, CompactionConfigStorage, CompactionJobGenerator, TabletManagementParametersTest, Manager, CancelCompactions, and PreDeleteTable
  • AmpleConditionalWriterIT.testCompacted() reordering of FateIds

@keith-turner
Copy link
Contributor

Changed compactionHints in TabletManagementParameters from Map<FateId,Map<String,String>> --> Map<String,Map<String,String>>: required changes to TabletManagementParameters, CompactionConfigStorage, CompactionJobGenerator, TabletManagementParametersTest, Manager, CancelCompactions, and PreDeleteTable

Sorry I did not communicate very clearly for the comment about that. Was not looking to change the external map type parameters for TabletManagementParameters to have a key type of String. Just thought it would be good to change internal map used for json so that the json encoding would cleaner. So was thinking of pattern like the following.

  • The public methods on TabletManagementParameters use Map<FateId,Map<String,String>>
  • Internally for json encoding Map<FateId,Map<String,String>> is converted to/from Map<String,Map<String,String>>

@kevinrr888
Copy link
Member Author

Changed compactionHints in TabletManagementParameters from Map<FateId,Map<String,String>> --> Map<String,Map<String,String>>: required changes to TabletManagementParameters, CompactionConfigStorage, CompactionJobGenerator, TabletManagementParametersTest, Manager, CancelCompactions, and PreDeleteTable

Sorry I did not communicate very clearly for the comment about that. Was not looking to change the external map type parameters for TabletManagementParameters to have a key type of String. Just thought it would be good to change internal map used for json so that the json encoding would cleaner. So was thinking of pattern like the following.

  • The public methods on TabletManagementParameters use Map<FateId,Map<String,String>>
  • Internally for json encoding Map<FateId,Map<String,String>> is converted to/from Map<String,Map<String,String>>

Ah gotcha. Will fix.

@kevinrr888
Changed TabletManagementParameters as outlined. Restored changes made…
fc6cc4b 
… to CompactionConfigStorage, CompactionJobGenerator, TabletManagementParametersTest, Manager, CancelCompactions, and PreDeleteTable from the previous commit involving changes to 'compactionHints'
@keith-turner keith-turner merged commit f8ab378 into apache:elasticity Feb 14, 2024
8 checks passed
@keith-turner keith-turner mentioned this pull request Feb 14, 2024
Merged
@dlmarion
Copy link
Contributor

dlmarion commented Feb 15, 2024 •
edited
Loading

@kevinrr888 - The latest IT run is failing (stacktrace below). I think it might be due to this merge. Just an FYI...

org.opentest4j.AssertionFailedError: Test json should be identical to actual metadata at this point ==> expected: 
<{"fateId":"FATE:USER:0000000000000002","selAll":true,"files":["
{\"path\":\"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/F0000070.rf\",\"startRow\":\"\",\"endRow\":\"\"}","
{\"path\":\"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/F0000071.rf\",\"startRow\":\"\",\"endRow\":\"\"}","
{\"path\":\"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/F0000072.rf\",\"startRow\":\"\",\"endRow\":\"\"}"]}> 
but was: <{"txid":"FATE:USER:0000000000000002","selAll":true,"files":["
{\"path\":\"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/F0000070.rf\",\"startRow\":\"\",\"endRow\":\"\"}","
{\"path\":\"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/F0000071.rf\",\"startRow\":\"\",\"endRow\":\"\"}","
{\"path\":\"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/F0000072.rf\",\"startRow\":\"\",\"endRow\":\"\"}"]}>

kevinrr888 added a commit to kevinrr888/accumulo that referenced this pull request Feb 15, 2024
@kevinrr888
Bug fix from PR apache#4247
d21eb50
kevinrr888 added a commit to kevinrr888/accumulo that referenced this pull request Feb 15, 2024
@kevinrr888
Bug fix from PR apache#4247
f0d2fbc
@kevinrr888 kevinrr888 mentioned this pull request Feb 15, 2024
Merged
@kevinrr888
Copy link
Member Author

@kevinrr888 - The latest IT run is failing (stacktrace below). I think it might be due to this merge. Just an FYI...

org.opentest4j.AssertionFailedError: Test json should be identical to actual metadata at this point ==> expected: 
<{"fateId":"FATE:USER:0000000000000002","selAll":true,"files":["
{\"path\":\"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/F0000070.rf\",\"startRow\":\"\",\"endRow\":\"\"}","
{\"path\":\"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/F0000071.rf\",\"startRow\":\"\",\"endRow\":\"\"}","
{\"path\":\"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/F0000072.rf\",\"startRow\":\"\",\"endRow\":\"\"}"]}> 
but was: <{"txid":"FATE:USER:0000000000000002","selAll":true,"files":["
{\"path\":\"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/F0000070.rf\",\"startRow\":\"\",\"endRow\":\"\"}","
{\"path\":\"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/F0000071.rf\",\"startRow\":\"\",\"endRow\":\"\"}","
{\"path\":\"hdfs://localhost:8020/accumulo/tables/2a/default_tablet/F0000072.rf\",\"startRow\":\"\",\"endRow\":\"\"}"]}>

Thanks for pointing this out. I created #4265

keith-turner pushed a commit that referenced this pull request Feb 15, 2024
@kevinrr888
Bug fix from PR #4247 (#4265)
10d0824
This was referenced Feb 16, 2024
Closed
Closed
Closed
@DomGarguilo DomGarguilo added this to the 4.0.0 milestone Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keith-turner keith-turner keith-turner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@kevinrr888 @keith-turner @dlmarion @DomGarguilo