You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/05/27 03:31:00 UTC
[GitHub] [hbase] virajjasani opened a new pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
virajjasani opened a new pull request #1784:
URL: https://github.com/apache/hbase/pull/1784
…regions
----------------------------------------------------------------
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
[GitHub] [hbase] anoopsjohn commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
anoopsjohn commented on a change in pull request #1784:
URL: https://github.com/apache/hbase/pull/1784#discussion_r431027896
##########
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:
@virajjasani One Q.
Rather than handling at this layer by checking whether the call is after a split or not, can we set a higher priority on the compact request, created after the split op? So then we dont need to set with these split status etc and handle specially at this layer.
----------------------------------------------------------------
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
[GitHub] [hbase] virajjasani merged pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
virajjasani merged pull request #1784:
URL: https://github.com/apache/hbase/pull/1784
----------------------------------------------------------------
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
[GitHub] [hbase] Apache-HBase commented on pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1784:
URL: https://github.com/apache/hbase/pull/1784#issuecomment-634664658
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 34s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 46s | master passed |
| +1 :green_heart: | compile | 0m 54s | master passed |
| +1 :green_heart: | shadedjars | 5m 31s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 38s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 27s | the patch passed |
| +1 :green_heart: | compile | 0m 56s | the patch passed |
| +1 :green_heart: | javac | 0m 55s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 33s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 37s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 154m 26s | hbase-server in the patch failed. |
| | | 178m 36s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1784 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux c26b95bc34cb 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 476cb16232 |
| Default Java | 1.8.0_232 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/2/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/2/testReport/ |
| Max. process+thread count | 3775 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1784:
URL: https://github.com/apache/hbase/pull/1784#issuecomment-634180775
----------------------------------------------------------------
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
[GitHub] [hbase] Apache-HBase commented on pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1784:
URL: https://github.com/apache/hbase/pull/1784#issuecomment-634660259
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 33s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 27s | master passed |
| +1 :green_heart: | compile | 1m 8s | master passed |
| +1 :green_heart: | shadedjars | 5m 52s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 41s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 7s | the patch passed |
| +1 :green_heart: | compile | 1m 4s | the patch passed |
| +1 :green_heart: | javac | 1m 4s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 14s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 50s | hbase-server in the patch failed. |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 143m 53s | hbase-server in the patch passed. |
| | | 170m 58s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1784 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 7fe61d84cca8 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 476cb16232 |
| Default Java | 2020-01-14 |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/2/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/2/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/2/testReport/ |
| Max. process+thread count | 3860 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] apurtell commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by 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
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1784:
URL: https://github.com/apache/hbase/pull/1784#issuecomment-634739859
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 9s | Docker mode activated. |
| -1 :x: | patch | 0m 3s | https://github.com/apache/hbase/pull/1784 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/in-progress/precommit-patchnames for help. |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1784 |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/3/console |
| versions | git=2.17.1 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #1784:
URL: https://github.com/apache/hbase/pull/1784#issuecomment-634738082
Thanks @apurtell @anoopsjohn for the review. Let me merge this.
----------------------------------------------------------------
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
[GitHub] [hbase] virajjasani commented on pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #1784:
URL: https://github.com/apache/hbase/pull/1784#issuecomment-634687168
Test failed in the recent build is flaky. It's passing locally.
----------------------------------------------------------------
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
[GitHub] [hbase] Apache-HBase commented on pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1784:
URL: https://github.com/apache/hbase/pull/1784#issuecomment-634590254
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 32s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 56s | master passed |
| +1 :green_heart: | checkstyle | 1m 8s | master passed |
| +1 :green_heart: | spotbugs | 2m 6s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 26s | the patch passed |
| +1 :green_heart: | checkstyle | 1m 12s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 11m 37s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 2m 15s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 16s | The patch does not generate ASF License warnings. |
| | | 33m 39s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1784 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux 66e1d5f0d67e 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 476cb16232 |
| Max. process+thread count | 94 (vs. ulimit of 12500) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1784/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1784: HBASE-24428 : Update compaction priority for recently split daughter …
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1784:
URL: https://github.com/apache/hbase/pull/1784#issuecomment-634740204
----------------------------------------------------------------
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