[GitHub] [accumulo] keith-turner commented on a diff in pull request #3249: Avoids holding tablet lock while doing split computations

["keith-turner (via GitHub)" <gi...@apache.org>]

keith-turner commented on code in PR #3249:
URL: https://github.com/apache/accumulo/pull/3249#discussion_r1148113852


##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java:
##########
@@ -1664,85 +1670,75 @@ private SplitRowSpec findSplitRow(Collection<FileRef> files) {
     }
 
     // check to see if one row takes up most of the tablet, in which case we can not split
-    try {
-
-      Text lastRow;
-      if (extent.getEndRow() == null) {
-        Key lastKey = (Key) FileUtil.findLastKey(getTabletServer().getFileSystem(),
-            getTabletServer().getConfiguration(), files);
-        lastRow = lastKey.getRow();
-      } else {
-        lastRow = extent.getEndRow();
-      }
-
-      // We expect to get a midPoint for this set of files. If we don't get one, we have a problem.
-      final Key mid = keys.get(.5);
-      if (mid == null) {
-        throw new IllegalStateException("Could not determine midpoint for files on " + extent);
-      }
+    Text lastRow;
+    if (extent.getEndRow() == null) {
+      lastRow = splitComputations.get().lastRowForDefaultTablet;
+    } else {
+      lastRow = extent.getEndRow();
+    }
 
-      // check to see that the midPoint is not equal to the end key
-      if (mid.compareRow(lastRow) == 0) {
-        if (keys.firstKey() < .5) {
-          Key candidate = keys.get(keys.firstKey());
-          if (candidate.getLength() > maxEndRow) {
-            log.warn("Cannot split tablet " + extent
-                + ", selected split point too long.  Length :  " + candidate.getLength());
+    // We expect to get a midPoint for this set of files. If we don't get one, we have a problem.
+    final Key mid = keys.get(.5);
+    if (mid == null) {
+      throw new IllegalStateException("Could not determine midpoint for files on " + extent);
+    }
 
-            sawBigRow = true;
-            timeOfLastMinCWhenBigFreakinRowWasSeen = lastMinorCompactionFinishTime;
-            timeOfLastImportWhenBigFreakinRowWasSeen = lastMapFileImportTime;
+    // check to see that the midPoint is not equal to the end key
+    if (mid.compareRow(lastRow) == 0) {
+      if (keys.firstKey() < .5) {
+        Key candidate = keys.get(keys.firstKey());
+        if (candidate.getLength() > maxEndRow) {
+          log.warn("Cannot split tablet " + extent + ", selected split point too long.  Length :  "
+              + candidate.getLength());
 
-            return null;
-          }
-          if (candidate.compareRow(lastRow) != 0) {
-            // we should use this ratio in split size estimations
-            if (log.isTraceEnabled())
-              log.trace(
-                  String.format("Splitting at %6.2f instead of .5, row at .5 is same as end row%n",
-                      keys.firstKey()));
-            return new SplitRowSpec(keys.firstKey(), candidate.getRow());
-          }
+          sawBigRow = true;
+          timeOfLastMinCWhenBigFreakinRowWasSeen = lastMinorCompactionFinishTime;
+          timeOfLastImportWhenBigFreakinRowWasSeen = lastMapFileImportTime;
 
+          return null;
+        }
+        if (candidate.compareRow(lastRow) != 0) {
+          // we should use this ratio in split size estimations
+          if (log.isTraceEnabled())
+            log.trace(
+                String.format("Splitting at %6.2f instead of .5, row at .5 is same as end row%n",
+                    keys.firstKey()));
+          return new SplitRowSpec(keys.firstKey(), candidate.getRow());
         }
 
-        log.warn("Cannot split tablet " + extent + " it contains a big row : " + lastRow);
+      }
 
-        sawBigRow = true;
-        timeOfLastMinCWhenBigFreakinRowWasSeen = lastMinorCompactionFinishTime;
-        timeOfLastImportWhenBigFreakinRowWasSeen = lastMapFileImportTime;
+      log.warn("Cannot split tablet " + extent + " it contains a big row : " + lastRow);

Review Comment:
   Gave this a try and it did not work.  Looked into why and it seems the Tablet class is using log4j directly for logging and not slf4j.   The lack of `{}` is a widespread issue in this class so I am not going to address that in this PR.  Hopefully things are better when I merge forward to 2.1



-- 
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