Skip to content

Update elasticity MergeTablets to use MERGED marker#3975

Merged
cshannon merged 3 commits into
apache:elasticityfrom
cshannon:elasticity-merge-marker
Nov 28, 2023
Merged

Update elasticity MergeTablets to use MERGED marker#3975
cshannon merged 3 commits into
apache:elasticityfrom
cshannon:elasticity-merge-marker

Conversation

@cshannon
Copy link
Copy Markdown
Contributor

@cshannon cshannon commented Nov 21, 2023

The MERGED marker is set on the last tablet of the merged range after metadata has been updated so that it is possible to know whether or not the files were already fenced. If the marker exists then the fencing for the last tablet can be skipped as the process was restarted.

This is a follow on to #3957 and applies that changes that were done in main in TabletGroupWatcher to the MergeTablets and DeleteTablets fate ops.

The existing tests in MergeIT verify that the merged marker is properly cleared.

@cshannon
Update elasticity MergeTablets to use MERGED marker
22a896c 
The MERGED marker is set on the last tablet of the merged range after
metadata has been updated so that it is possible to know whether or not
the files were already fenced. If the marker exists then the fencing for
the last tablet can be skipped as the process was restarted.
@cshannon cshannon self-assigned this Nov 21, 2023
@cshannon
Copy link
Copy Markdown
Contributor Author

cshannon commented Nov 21, 2023

@keith-turner - I put the code to clean up the MERGED marker in DeleteTablets because this seemed like the simplest place to put it. I could also move it into FinishTableRangeOp but that currently can be called even if there is no marker to clean up (such as if only 1 tablet for the merge range and it skips) where as DeleteTablets should only be called if there is a MERGED marker to clean up. But I can move the clean up there if you think that makes more sense and just check if the marker exists before removing it or just remove it regardless (I don't think there's any harm in deleting a column that doesn't exist)

keith-turner
keith-turner approved these changes Nov 27, 2023
View reviewed changes
Comment thread core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java
assertEquals(159L, tm3.getSelectedFiles().getFateTxId());
assertFalse(tm3.getSelectedFiles().initiallySelectedAll());
assertEquals(selFiles.getMetadataValue(), tm3.getSelectedFiles().getMetadataValue());
assertTrue(tm3.hasMerged());
Copy link
Copy Markdown
Contributor

@keith-turner keith-turner Nov 27, 2023

Choose a reason for hiding this comment

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

Would be good to add another check somewhere else for the case when merged is not set to enure its false.

Copy link
Copy Markdown
Contributor Author

@cshannon cshannon Nov 28, 2023

Choose a reason for hiding this comment

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

I will add an assertion for that above to one of the other TabletMetadata objects that doesn't set it

@cshannon cshannon merged commit c615abf into apache:elasticity Nov 28, 2023
@cshannon cshannon deleted the elasticity-merge-marker branch December 3, 2023 15:02
@cshannon cshannon mentioned this pull request Jun 7, 2024
Merged
@ctubbsii ctubbsii 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

@cshannon cshannon

Labels

None yet

Projects

Accumulo: Elasticity
Status: ✅ Done

Milestone

4.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@cshannon @keith-turner @ctubbsii