You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2023/11/11 20:06:07 UTC

[PR] adds support for compacting lots of tablets [accumulo]

keith-turner opened a new pull request, #3945:
URL: https://github.com/apache/accumulo/pull/3945

   Removes buffering of all conditional results in memory in the compaction code.  This allows compacting more tablets than would fit in memory.
   
   Changed the condition result BiConsumer to a Consumer because it was passing the extents twice, so a BiConsumer was not needed and made the code more verbose.
   
   Updated the compaction code to collect stats and log a trace.  It was logging a lot of per tablet information at debug.  When compacting one million tablets, this resulted in a lot of information in the manager logs.  Moved the per tablet information to trace logging.  Added collection of stats for the different per tablet information and logged the stats once for all tablets scanned.
   
   Added compaction to the SplitMillionIT. Without the other changes in this PR adding compaction to the SplitMillion would cause the Manager to die with an out of memory error because the conditional write was buffering all 1 million tablets.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] adds support for compacting lots of tablets [accumulo]

Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3945:
URL: https://github.com/apache/accumulo/pull/3945#discussion_r1394367477


##########
test/src/main/java/org/apache/accumulo/test/functional/SplitMillionIT.java:
##########
@@ -42,11 +49,20 @@
 
 public class SplitMillionIT extends AccumuloClusterHarness {
 
+  private static final Logger log = LoggerFactory.getLogger(SplitIT.class);

Review Comment:
   done in b573f09



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] adds support for compacting lots of tablets [accumulo]

Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3945:
URL: https://github.com/apache/accumulo/pull/3945#discussion_r1394367789


##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java:
##########
@@ -137,45 +140,60 @@ public int updateAndCheckTablets(Manager manager, long tid)
 
     // ELASTICITY_TODO use existing compaction logging
 
-    try (
-        var tablets = ample.readTablets().forTable(tableId).overlapping(startRow, endRow)
-            .fetch(PREV_ROW, COMPACTED, FILES, SELECTED, ECOMP, OPID).checkConsistency().build();
-        var tabletsMutator = ample.conditionallyMutateTablets()) {
+    var fateStr = FateTxId.formatTid(tid);
+
+    Consumer<Ample.ConditionalResult> resultConsumer = result -> {
+      if (result.getStatus() == Status.REJECTED) {
+        log.debug("{} update for {} was rejected ", fateStr, result.getExtent());
+      }
+    };
+
+    long t1 = System.currentTimeMillis();
 
-      int complete = 0;
-      int total = 0;
+    int complete = 0;
+    int total = 0;
+    int opidsSeen = 0;
+    int noFiles = 0;
+    int noneSelected = 0;
+    int alreadySelected = 0;
+    int otherSelected = 0;
+    int otherCompaction = 0;
+    int selected = 0;
 
-      int selected = 0;
+    KeyExtent minSelected = null;
+    KeyExtent maxSelected = null;
 
-      KeyExtent minSelected = null;
-      KeyExtent maxSelected = null;
+    try (
+        var tablets = ample.readTablets().forTable(tableId).overlapping(startRow, endRow)
+            .fetch(PREV_ROW, COMPACTED, FILES, SELECTED, ECOMP, OPID).checkConsistency().build();
+        var tabletsMutator = ample.conditionallyMutateTablets(resultConsumer)) {
 
       CompactionConfig config = CompactionConfigStorage.getConfig(manager.getContext(), tid);
 
       for (TabletMetadata tablet : tablets) {
 
         total++;
 
-        // TODO change all logging to trace
-
         if (tablet.getCompacted().contains(tid)) {
           // this tablet is already considered done
-          log.debug("{} compaction for {} is complete", FateTxId.formatTid(tid),
+          log.trace("{} compaction for {} is complete", FateTxId.formatTid(tid),

Review Comment:
   done in b573f09



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] adds support for compacting lots of tablets [accumulo]

Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3945:
URL: https://github.com/apache/accumulo/pull/3945#discussion_r1392953913


##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java:
##########
@@ -137,45 +140,60 @@ public int updateAndCheckTablets(Manager manager, long tid)
 
     // ELASTICITY_TODO use existing compaction logging
 
-    try (
-        var tablets = ample.readTablets().forTable(tableId).overlapping(startRow, endRow)
-            .fetch(PREV_ROW, COMPACTED, FILES, SELECTED, ECOMP, OPID).checkConsistency().build();
-        var tabletsMutator = ample.conditionallyMutateTablets()) {
+    var fateStr = FateTxId.formatTid(tid);
+
+    Consumer<Ample.ConditionalResult> resultConsumer = result -> {
+      if (result.getStatus() == Status.REJECTED) {
+        log.debug("{} update for {} was rejected ", fateStr, result.getExtent());
+      }
+    };
+
+    long t1 = System.currentTimeMillis();
 
-      int complete = 0;
-      int total = 0;
+    int complete = 0;
+    int total = 0;
+    int opidsSeen = 0;
+    int noFiles = 0;
+    int noneSelected = 0;
+    int alreadySelected = 0;
+    int otherSelected = 0;
+    int otherCompaction = 0;
+    int selected = 0;
 
-      int selected = 0;
+    KeyExtent minSelected = null;
+    KeyExtent maxSelected = null;
 
-      KeyExtent minSelected = null;
-      KeyExtent maxSelected = null;
+    try (
+        var tablets = ample.readTablets().forTable(tableId).overlapping(startRow, endRow)
+            .fetch(PREV_ROW, COMPACTED, FILES, SELECTED, ECOMP, OPID).checkConsistency().build();
+        var tabletsMutator = ample.conditionallyMutateTablets(resultConsumer)) {
 
       CompactionConfig config = CompactionConfigStorage.getConfig(manager.getContext(), tid);
 
       for (TabletMetadata tablet : tablets) {
 
         total++;
 
-        // TODO change all logging to trace
-
         if (tablet.getCompacted().contains(tid)) {
           // this tablet is already considered done
-          log.debug("{} compaction for {} is complete", FateTxId.formatTid(tid),
+          log.trace("{} compaction for {} is complete", FateTxId.formatTid(tid),

Review Comment:
   I think you can replace all calls to `FateTxId.formatTid(tid)` with the variable `fateStr` from above.



##########
test/src/main/java/org/apache/accumulo/test/functional/SplitMillionIT.java:
##########
@@ -42,11 +49,20 @@
 
 public class SplitMillionIT extends AccumuloClusterHarness {
 
+  private static final Logger log = LoggerFactory.getLogger(SplitIT.class);

Review Comment:
   ```suggestion
     private static final Logger log = LoggerFactory.getLogger(SplitMillionIT.class);
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] adds support for compacting lots of tablets [accumulo]

Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #3945:
URL: https://github.com/apache/accumulo/pull/3945


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org