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:
[email protected]
