Skip to content

Globally Unique FATE Transaction Ids - Part 3#4247

Merged
keith-turner merged 4 commits into
apache:elasticityfrom
kevinrr888:elasticity-feature-4044-deferrals
Feb 14, 2024
Merged

Globally Unique FATE Transaction Ids - Part 3#4247
keith-turner merged 4 commits into
apache:elasticityfrom
kevinrr888:elasticity-feature-4044-deferrals

Conversation

@kevinrr888
Copy link
Copy Markdown
Member

@kevinrr888 kevinrr888 commented Feb 8, 2024

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
Comment thread core/src/main/java/org/apache/accumulo/core/fate/FateId.java Outdated
Comment thread core/src/main/java/org/apache/accumulo/core/fate/FateId.java Outdated
Comment thread core/src/main/java/org/apache/accumulo/core/metadata/schema/CompactionMetadata.java
String groupId;
short priority;
boolean propDels;
Long fateTxId;
Copy link
Copy Markdown
Contributor

@keith-turner keith-turner Feb 13, 2024

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

Comment thread core/src/main/java/org/apache/accumulo/core/metadata/schema/CompactionMetadata.java Outdated
Comment thread core/src/main/java/org/apache/accumulo/core/metadata/schema/CompactionMetadata.java Outdated
Comment thread server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionConfigStorage.java
Comment thread .../base/src/main/java/org/apache/accumulo/server/manager/state/TabletManagementParameters.java Outdated
Comment thread test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java Outdated
Comment thread test/src/main/java/org/apache/accumulo/test/functional/SplitRecoveryIT.java
Comment thread ...r/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/CleanUpBulkImport.java
@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
Copy Markdown
Member Author

kevinrr888 commented Feb 14, 2024

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
Copy Markdown
Contributor

keith-turner commented Feb 14, 2024

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
Copy Markdown
Member Author

kevinrr888 commented Feb 14, 2024

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
@keith-turner keith-turner mentioned this pull request Feb 14, 2024
Merged
@dlmarion
Copy link
Copy Markdown
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
Copy Markdown
Member Author

kevinrr888 commented Feb 15, 2024

@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