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.

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

speed up merge operation for lots of tablets #4574

Merged
merged 6 commits into from
May 24, 2024

Conversation

keith-turner
Copy link
Contributor

Made two changes to speed up merge. First adjusted the sleep time for one of the Fate steps to consider how long a scan took. For SplitMillionIT this resulted in ReserveTablets.isReady() sleeping for
around 30s instead of 60s. Second when the merge code was deleting
tablets it was asking TabletMetadata to save key values and those saved
key values were being sorted, even though they did not need to be and
were actually already sorted. Changed code to not sort and the resulted
in DeleteTablets.call() going from 63s to 55s. Below are the relevant
log messages from before and after runs w/ change to not sort.

2024-05-17T18:27:49,635 104 [fate.Fate] DEBUG: Running DeleteTablets.call() FATE:USER:9b3c9abe-a29a-4776-80ee-b0e6c0132d98 took 63328 ms and returned FinishTableRangeOp
2024-05-17T20:20:31,468 103 [fate.Fate] DEBUG: Running DeleteTablets.call() FATE:USER:3f566052-d88d-44dd-9e8b-ac34ed9ca445 took 54710 ms and returned FinishTableRangeO

Made two changes to speed up merge. First adjusted the sleep time for
one of the Fate steps to consider how long a scan took.  For
SplitMillionIT this resulted in ReserveTablets.isReady() sleeping for
around 30s instead of 60s.   Second when the merge code was deleting
tablets it was asking TabletMetadata to save key values and those saved
key values were being sorted, even though they did not need to be and
were actually already sorted. Changed code to not sort and the resulted
in DeleteTablets.call() going from 63s to 55s.  Below are the relevant
log messages from before and after runs w/ change to not sort.

```
2024-05-17T18:27:49,635 104 [fate.Fate] DEBUG: Running DeleteTablets.call() FATE:USER:9b3c9abe-a29a-4776-80ee-b0e6c0132d98 took 63328 ms and returned FinishTableRangeOp
```

```
2024-05-17T20:20:31,468 103 [fate.Fate] DEBUG: Running DeleteTablets.call() FATE:USER:3f566052-d88d-44dd-9e8b-ac34ed9ca445 took 54710 ms and returned FinishTableRangeO
```
Copy link
Contributor

@cshannon cshannon left a comment

Choose a reason for hiding this comment

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

@keith-turner - What do you think about moving part of these changes to main? Specifically, just the Ample changes that change the keyValues from being a SortedMap to a List ? All of that exists in 3.1 so it would things easier going forward if the API was the same in both when merging forward for things like deleteAll()

@keith-turner
Copy link
Contributor Author

@keith-turner - What do you think about moving part of these changes to main?

Looked into the usage in each branch. The following is for main.

$ find . -name "*.java" | xargs grep getKeyValues 
./server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java:      bw.addMutation(createCloneMutation(srcTableId, tableId, ti.next().getKeyValues()));
./server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java:        for (Entry<Key,Value> entry : cloneTablet.getKeyValues().entrySet()) {
./server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java:          bw.addMutation(createCloneMutation(srcTableId, tableId, st.getKeyValues()));
./core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:    assertEquals(rowMap, tm.getKeyValues());
./core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:    // Verify the various collections are immutable and non-null (except for getKeyValues) if
./core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:    assertThrows(IllegalStateException.class, tm::getKeyValues);
./core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:    assertEquals(1, tm2.getKeyValues().size());
./core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:        () -> tm2.getKeyValues().put(new Key(), new Value()));
./core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:  public SortedMap<Key,Value> getKeyValues() {

and the following if for elasticity ommitting a few false positives related to RootTableMetadata which has function w/. the same name

$ find . -name "*.java" | xargs grep getKeyValues
./server/manager/src/main/java/org/apache/accumulo/manager/tableOps/merge/DeleteTablets.java:          tabletMeta.getKeyValues().keySet().forEach(key -> {
./server/manager/src/main/java/org/apache/accumulo/manager/tableOps/merge/DeleteTablets.java:        tabletMutator.deleteAll(tabletMeta.getKeyValues().keySet());
./server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader11to12.java:      rtm.getKeyValues().filter(e -> UPGRADE_FAMILIES.contains(e.getKey().getColumnFamily()))
./server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java:        bw.addMutation(createCloneMutation(srcTableId, tableId, ti.next().getKeyValues()));
./server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java:          for (Entry<Key,Value> entry : cloneTablet.getKeyValues().entrySet()) {
./server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java:            bw.addMutation(createCloneMutation(srcTableId, tableId, st.getKeyValues()));
./server/base/src/test/java/org/apache/accumulo/server/SetEncodingIteratorTest.java:    SortedMap<Key,Value> sortedMapNoFiles = new TreeMap<>(tmNoFiles.getKeyValues());
./server/base/src/test/java/org/apache/accumulo/server/SetEncodingIteratorTest.java:    SortedMap<Key,Value> sortedMapOneFile = new TreeMap<>(tmOneFile.getKeyValues());
./server/base/src/test/java/org/apache/accumulo/server/SetEncodingIteratorTest.java:    SortedMap<Key,Value> sortedMap = new TreeMap<>(tmMultipleFiles.getKeyValues());
./server/base/src/test/java/org/apache/accumulo/server/SetEncodingIteratorTest.java:    SortedMap<Key,Value> sortedMap2 = new TreeMap<>(tmMultipleFiles2.getKeyValues());
./server/base/src/test/java/org/apache/accumulo/server/manager/state/TabletManagementTest.java:    assertEquals(entries, tmi.getTabletMetadata().getKeyValues());
./server/base/src/test/java/org/apache/accumulo/server/manager/state/TabletManagementTest.java:    assertEquals(entries, tmi.getTabletMetadata().getKeyValues());
./server/base/src/test/java/org/apache/accumulo/server/manager/state/TabletManagementTest.java:    assertEquals(entries, tmi.getTabletMetadata().getKeyValues());
./test/src/main/java/org/apache/accumulo/test/functional/SplitIT.java:      var original = new TreeMap<>(tabletMetadata.getKeyValues());
./test/src/main/java/org/apache/accumulo/test/functional/SplitIT.java:      var kvCopy = new TreeMap<>(tabletMetadata2.getKeyValues());
./core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:    assertEquals(rowMap, tm.getKeyValues());
./core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:    // Verify the various collections are immutable and non-null (except for getKeyValues) if
./core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:    assertThrows(IllegalStateException.class, tm::getKeyValues);
./core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:    assertEquals(1, tm2.getKeyValues().size());
./core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java:        () -> tm2.getKeyValues().put(new Key(), new Value()));
./core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:  public SortedMap<Key,Value> getKeyValues() {

Seems like the only overlap outside of test is the clone table code, the merge code does not use it in main. The function is used much less in main. Since its used much less in main, would be easy to make the change in main. When merging forward would need to make a lot of the changes in this PR in the merge commit so it would compile. I am fine w/ making the change in main, I am puzzling over what the path of least work is though and if there is some git magic I can leverage.

@cshannon
Copy link
Contributor

If it ends up not being worth making the change in main that is fine too, I just figured I'd bring it up since I noticed it was in main too

keith-turner added a commit to keith-turner/accumulo that referenced this pull request May 23, 2024
This change was made in elasticity as part of a larger change
to speedup merge, see apache#4574.  Backported just the part that
replaced a sortedmap w/ a list in TabletMetadata to speed up
the code that tracks a tablets key/values.
@keith-turner
Copy link
Contributor Author

@cshannon opened #4600. When that is merged, can merge it to elasticity and then into this.

keith-turner added a commit that referenced this pull request May 24, 2024
This change was made in elasticity as part of a larger change
to speedup merge, see #4574.  Backported just the part that
replaced a sortedmap w/ a list in TabletMetadata to speed up
the code that tracks a tablets key/values.
@keith-turner
Copy link
Contributor Author

A lot of the changes that were in this PR are now in the commits 219d4f2 and ef213b5

@keith-turner keith-turner merged commit 7af0dec into apache:elasticity May 24, 2024
8 checks passed
@keith-turner keith-turner deleted the merge-speedup branch May 24, 2024 17:47
keith-turner added a commit to keith-turner/accumulo that referenced this pull request May 24, 2024
The fate step that reserves tablets for merge was signaling it
was ready when it was not in the case where its scan of the
metadata table took 0ms.  This was causing some ITs to fail.  The
bug was introduced in apache#4574
keith-turner added a commit that referenced this pull request May 24, 2024
The fate step that reserves tablets for merge was signaling it
was ready when it was not in the case where its scan of the
metadata table took 0ms.  This was causing some ITs to fail.  The
bug was introduced in #4574
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants