[GitHub] [accumulo] dlmarion commented on a diff in pull request #3221: Do not calculate split point in Tablet.needsSplit() (#3217)

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

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


##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java:
##########
@@ -1624,6 +1624,16 @@ private SplitRowSpec findSplitRow(Collection<FileRef> files) {
     long maxEndRow = tableConfiguration.getMemoryInBytes(Property.TABLE_MAX_END_ROW_SIZE);
 
     if (extent.isRootTablet() || estimateTabletSize() <= splitThreshold) {
+      return false;

Review Comment:
   I believe you can remove the line above that sets the variable `maxEndRow`.



##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java:
##########
@@ -1814,7 +1824,7 @@ List<FileRef> findChopFiles(KeyExtent extent, Map<FileRef,Pair<Key,Key>> firstAn
   public synchronized boolean needsSplit() {
     if (isClosing() || isClosed())
       return false;
-    return findSplitRow(getDatafileManager().getFiles()) != null;
+    return isSplitPossible();

Review Comment:
   `findSplitRow` checks more conditions than `isSplitPossible`. Would a side-effect of this change be that more tablets are queued up to be split even though they might not need it (see Tablet.importMapFiles())? I wonder if the checks that are performed in `findSplitRow()` should be moved to a place where they are performed after the Tablet is modified, say post-majc, and then the results of that just stored in a variable that is checked when `needsSplit` is called.



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