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

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

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


##########
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:
   should we use logging placeholders ("{}") here? Given that it's at the warn level, I'm not sure it makes a difference. Just a thought.



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