[GitHub] [accumulo] keith-turner commented on a diff in pull request #2804: Modify CompactionCheck logic to run expensive checks less often

[GitBox <gi...@apache.org>]

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


##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1105,23 +1105,23 @@ public Optional<Files> getFiles(CompactionServiceId service, CompactionKind kind
   }
 
   class CompactionCheck {
-    private final Supplier<Boolean> memoizedCheck;
+    private final Supplier<Boolean> expensiveCheck;
+    private final Supplier<Boolean> inexpensiveCheck;
 
     public CompactionCheck(CompactionServiceId service, CompactionKind kind, Long compactionId) {
-      this.memoizedCheck = Suppliers.memoizeWithExpiration(() -> {
-        if (closed)
+      this.expensiveCheck = Suppliers.memoizeWithExpiration(() -> {
+        return service.equals(getConfiguredService(kind));
+      }, 3, TimeUnit.SECONDS);
+      this.inexpensiveCheck = Suppliers.memoizeWithExpiration(() -> {
+        if (closed
+            || (kind == CompactionKind.USER && lastSeenCompactionCancelId.get() >= compactionId))
           return false;
-        if (!service.equals(getConfiguredService(kind)))
-          return false;
-        if (kind == CompactionKind.USER && lastSeenCompactionCancelId.get() >= compactionId)
-          return false;
-
         return true;
-      }, 100, TimeUnit.MILLISECONDS);
+      }, 50, TimeUnit.MILLISECONDS);

Review Comment:
   Was there a specific reason to adjust this time from 100 to 50?  The initial 100ms time was arbitrarily chosen, so not in any way opposed to changing it.  Just curious if you saw something that prompted this. 



##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1105,23 +1105,23 @@ public Optional<Files> getFiles(CompactionServiceId service, CompactionKind kind
   }
 
   class CompactionCheck {
-    private final Supplier<Boolean> memoizedCheck;
+    private final Supplier<Boolean> expensiveCheck;
+    private final Supplier<Boolean> inexpensiveCheck;
 
     public CompactionCheck(CompactionServiceId service, CompactionKind kind, Long compactionId) {
-      this.memoizedCheck = Suppliers.memoizeWithExpiration(() -> {
-        if (closed)
+      this.expensiveCheck = Suppliers.memoizeWithExpiration(() -> {
+        return service.equals(getConfiguredService(kind));

Review Comment:
   How was it determined this check was expensive?



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