[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
virajjasani commented on a change in pull request #1784: URL: https://github.com/apache/hbase/pull/1784#discussion_r431054125 ## 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: There are 2 reasons: 1. `isAfterSplit` is an additional field which could be useful for maybe some other purpose also (in future). 2. We already have logic to determine priority at this layer only: `request.setPriority((priority != Store.NO_PRIORITY) ? priority : getCompactPriority());`. Nowhere other than this place, are we using priority setter `request.setPriority(int p)`. So this will provide better alignment and also avoid setting priority in multiple places: `SortedCompactionPolicy`, `StripeCompactionPolicy`, `HStore`. 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: us...@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
virajjasani commented on a change in pull request #1784: URL: https://github.com/apache/hbase/pull/1784#discussion_r431054125 ## 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: There are 2 reasons: 1. `isAfterSplit` is an additional field which could be useful for maybe some other purpose also (in future). 2. We already have logic to determine priority present here: `request.setPriority((priority != Store.NO_PRIORITY) ? priority : getCompactPriority());`. Nowhere other than this place, are we using priority setter `request.setPriority(int p)`. So this will provide better alignment and also avoid setting priority in multiple places: `SortedCompactionPolicy`, `StripeCompactionPolicy`, `HStore`. 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: us...@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
virajjasani commented on a change in pull request #1784: URL: https://github.com/apache/hbase/pull/1784#discussion_r431054125 ## 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: There are 2 reasons: 1. `isAfterSplit` is an additional field which could be useful for maybe some other info in future also. 2. We already have logic to determine priority present here: `request.setPriority((priority != Store.NO_PRIORITY) ? priority : getCompactPriority());`. Nowhere other than this place, are we using priority setter `request.setPriority(int p)`. So this will provide better alignment and also avoid setting priority in multiple places: `SortedCompactionPolicy`, `StripeCompactionPolicy`, `HStore`. 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: us...@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
virajjasani commented on a change in pull request #1784: URL: https://github.com/apache/hbase/pull/1784#discussion_r431054125 ## 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: There are 2 reasons: 1. `isAfterSplit` is an additional field which could be useful for maybe some other info in future also. 2. We already have logic to determine priority present here: `request.setPriority((priority != Store.NO_PRIORITY) ? priority : getCompactPriority());`. Nowhere other than this place, are we using priority setter `request.setPriority(int p)`. So this will provide better alignment and also avoid setting priority in multiple places: SortedCompactionPolicy, StripeCompactionPolicy. 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: us...@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
virajjasani commented on a change in pull request #1784: URL: https://github.com/apache/hbase/pull/1784#discussion_r431054125 ## 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: There are 2 reasons: 1. `isAfterSplit` is an additional field which could be useful for maybe some other info in future also. 2. We already have logic to determine priority present here: `request.setPriority((priority != Store.NO_PRIORITY) ? priority : getCompactPriority());`. Nowhere other than this place, are we using priority setter `request.setPriority(int p)`. So this will provide better alignment and also avoid setting priority in multiple places: `SortedCompactionPolicy`, `StripeCompactionPolicy`. 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: us...@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
virajjasani commented on a change in pull request #1784: URL: https://github.com/apache/hbase/pull/1784#discussion_r431054125 ## 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: There are 2 reasons: 1. `isAfterSplit` is an additional field which could be useful for maybe some other info in future also. 2. We already have logic to determine priority present here: `request.setPriority((priority != Store.NO_PRIORITY) ? priority : getCompactPriority());`. Nowhere other than this place, are we using priority setter `request.setPriority(int p)`. So this will provide better alignment. 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: us...@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
virajjasani commented on a change in pull request #1784: URL: https://github.com/apache/hbase/pull/1784#discussion_r430899478 ## 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: Agree, we should override priority only if it is higher than constant (MIN_VALUE + 1000). ## 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: Yeah right, I exactly looked for an existing boolean and applied same formula rather than regenerating hashcode() by IDE :) ## 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: Yeah, MIN_VALUE+1000 (or 5000) should be able to provide more than enough room for tasks that can emerge as even higher priority than split housekeeping in future. 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: us...@infra.apache.org