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

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on code in PR #3249:
URL: https://github.com/apache/accumulo/pull/3249#discussion_r1146983348


##########
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:
   Definitely should use placeholders whenever possible, for safety. You never know what's going to get logged and incorrectly parsed as part of the format string. We took efforts in 2.0 to fix a bunch of those, and we should try to fix it whenever we see them going forward.



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