[GitHub] [hbase] apurtell commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …

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

apurtell commented on a change in pull request #1784:
URL: https://github.com/apache/hbase/pull/1784#discussion_r430715327



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -1921,9 +1921,15 @@ public boolean shouldPerformMajorCompaction() throws IOException {
         // If we're enqueuing a major, clear the force flag.
         this.forceMajor = this.forceMajor && !request.isMajor();
 
-        // Set common request properties.
-        // Set priority, either override value supplied by caller or from store.
-        request.setPriority((priority != Store.NO_PRIORITY) ? priority : getCompactPriority());
+        if (request.isAfterSplit()) {

Review comment:
       Hmm. I suppose this is fine. We already have `request#isMajor` to inform us if we need to schedule a major compaction.
   
   `request#isSplit` is just as informative as `request#isAfterSplit`, but no strong opinion there. 

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -1921,9 +1921,15 @@ public boolean shouldPerformMajorCompaction() throws IOException {
         // If we're enqueuing a major, clear the force flag.
         this.forceMajor = this.forceMajor && !request.isMajor();
 
-        // Set common request properties.
-        // Set priority, either override value supplied by caller or from store.
-        request.setPriority((priority != Store.NO_PRIORITY) ? priority : getCompactPriority());
+        if (request.isAfterSplit()) {
+          // If the store belongs to recently splitted daughter regions, better we consider
+          // them with the highest priority in the compaction queue.
+          request.setPriority(Integer.MIN_VALUE);

Review comment:
       Or should we define a new constant for high priority splits that is not quite MIN_VALUE, so someone can still schedule something ahead of us? idk, like MIN_VALUE+1000. 

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -1921,9 +1921,15 @@ public boolean shouldPerformMajorCompaction() throws IOException {
         // If we're enqueuing a major, clear the force flag.
         this.forceMajor = this.forceMajor && !request.isMajor();
 
-        // Set common request properties.
-        // Set priority, either override value supplied by caller or from store.
-        request.setPriority((priority != Store.NO_PRIORITY) ? priority : getCompactPriority());
+        if (request.isAfterSplit()) {

Review comment:
       We should log when we are overriding request priority with the reason why, at DEBUG level at least, but INFO makes sense too.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/CompactionRequestImpl.java
##########
@@ -149,6 +158,7 @@ public int hashCode() {
     result = prime * result + ((storeName == null) ? 0 : storeName.hashCode());
     result = prime * result + (int) (totalSize ^ (totalSize >>> 32));
     result = prime * result + ((tracker == null) ? 0 : tracker.hashCode());
+    result = prime * result + (isAfterSplit ? 1231 : 1237);

Review comment:
       This pattern is not one I've seen before but we do it above for `isOffPeak` so (shrug). 

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -1921,9 +1921,15 @@ public boolean shouldPerformMajorCompaction() throws IOException {
         // If we're enqueuing a major, clear the force flag.
         this.forceMajor = this.forceMajor && !request.isMajor();
 
-        // Set common request properties.
-        // Set priority, either override value supplied by caller or from store.
-        request.setPriority((priority != Store.NO_PRIORITY) ? priority : getCompactPriority());
+        if (request.isAfterSplit()) {

Review comment:
       Oh, if you accept my above suggestion wrt a constant value for split housekeeping that is not quite MIN_VALUE then we should not override priority if it is already equal to or less than what we need.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org