Skip to content

HBASE-24931 Fix for Candidate generator getAction method ignoring 0th… #2293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

HBASE-24931 Fix for Candidate generator getAction method ignoring 0th… #2293

wants to merge 1 commit into from

Conversation

@rda3mon
Copy link
Contributor

@rda3mon rda3mon commented Aug 23, 2020

… index

@Apache-HBase
Copy link

Apache-HBase commented Aug 23, 2020

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 28s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
_ master Compile Tests _
+1 💚 mvninstall 3m 41s master passed
+1 💚 checkstyle 1m 5s master passed
+1 💚 spotbugs 1m 58s master passed
_ Patch Compile Tests _
+1 💚 mvninstall 3m 26s the patch passed
+1 💚 checkstyle 1m 1s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 hadoopcheck 11m 15s Patch does not cause any errors with Hadoop 3.1.2 3.2.1.
+1 💚 spotbugs 2m 7s the patch passed
_ Other Tests _
+1 💚 asflicense 0m 15s The patch does not generate ASF License warnings.
32m 25s
Subsystem Report/Notes
Docker Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2293/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #2293
Optional Tests dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle
uname Linux 70200f6d7cc5 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 / 9f62a82
Max. process+thread count 94 (vs. ulimit of 12500)
modules C: hbase-server U: hbase-server
Console output https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2293/1/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.

virajjasani
virajjasani reviewed Aug 23, 2020
View reviewed changes
Copy link
Contributor

@virajjasani virajjasani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rda3mon Thanks for the PR and considering region of 0th location sounds reasonable, however can you please provide the purpose behind this find? Is it that you observed this behaviour while generating plans?
Just trying to figure out if this was bug or intentional behaviour by any chance. Also, this part of the code is not updated since HBASE-10351. (which of course doesn't mean that this is not a valid bug)

@rda3mon
Copy link
Contributor Author

rda3mon commented Aug 23, 2020 •
edited
Loading

@virajjasani I noticed this bug while writing a custom Candidate generator for reassigning those regions which had one of their favorednodes as dead. Cost Multiplier was on the higher side, and hence it was always balancing while 1 region was never reassigned.
Blocks to dead favorednodes spill over to any other datanode without rsgroup barrier, which wasn't acceptable as wanted a tighter isolation

@Apache-HBase
Copy link

Apache-HBase commented Aug 23, 2020

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 25s Docker mode activated.
-0 ⚠️ yetus 0m 4s Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+1 💚 mvninstall 4m 19s master passed
+1 💚 compile 1m 6s master passed
+1 💚 shadedjars 5m 49s branch has no errors when building our shaded downstream artifacts.
-0 ⚠️ javadoc 0m 42s hbase-server in master failed.
_ Patch Compile Tests _
+1 💚 mvninstall 4m 1s the patch passed
+1 💚 compile 1m 3s the patch passed
+1 💚 javac 1m 3s the patch passed
+1 💚 shadedjars 5m 45s patch has no errors when building our shaded downstream artifacts.
-0 ⚠️ javadoc 0m 39s hbase-server in the patch failed.
_ Other Tests _
-1 ❌ unit 130m 2s hbase-server in the patch failed.
156m 1s
Subsystem Report/Notes
Docker Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2293/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR #2293
Optional Tests javac javadoc unit shadedjars compile
uname Linux 4254ce565be3 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 / 9f62a82
Default Java 2020-01-14
javadoc https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2293/1/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt
javadoc https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2293/1/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt
unit https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2293/1/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt
Test Results https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2293/1/testReport/
Max. process+thread count 3890 (vs. ulimit of 12500)
modules C: hbase-server U: hbase-server
Console output https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2293/1/console
versions git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f)
Powered by Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase
Copy link

Apache-HBase commented Aug 23, 2020

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 47s Docker mode activated.
-0 ⚠️ yetus 0m 2s Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+1 💚 mvninstall 4m 23s master passed
+1 💚 compile 1m 8s master passed
+1 💚 shadedjars 6m 49s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 0m 38s master passed
_ Patch Compile Tests _
+1 💚 mvninstall 3m 56s the patch passed
+1 💚 compile 1m 7s the patch passed
+1 💚 javac 1m 7s the patch passed
+1 💚 shadedjars 6m 30s patch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 0m 39s the patch passed
_ Other Tests _
-1 ❌ unit 149m 24s hbase-server in the patch failed.
177m 17s
Subsystem Report/Notes
Docker Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2293/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR #2293
Optional Tests javac javadoc unit shadedjars compile
uname Linux 73532a5fcfd0 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 / 9f62a82
Default Java 1.8.0_232
unit https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2293/1/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt
Test Results https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2293/1/testReport/
Max. process+thread count 3701 (vs. ulimit of 12500)
modules C: hbase-server U: hbase-server
Console output https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2293/1/console
versions git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f)
Powered by Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@rda3mon
Copy link
Contributor Author

rda3mon commented Aug 23, 2020

Fixing broken tests

virajjasani
virajjasani previously approved these changes Aug 23, 2020
View reviewed changes
Copy link
Contributor

@virajjasani virajjasani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, sounds reasonable.

@virajjasani
Copy link
Contributor

virajjasani commented Aug 23, 2020

@busbey would you like to take a look?

@virajjasani
Copy link
Contributor

virajjasani commented Aug 24, 2020

@rda3mon did you miss git push by any chance when you said "Fixing broken tests" ?

@rda3mon
Copy link
Contributor Author

rda3mon commented Aug 24, 2020

@virajjasani I need another day or so. It requires more involved understanding to fix broken tests. I will get back once I fix them

@virajjasani
Copy link
Contributor

virajjasani commented Aug 24, 2020

No worries, please take your time.
Thanks

@virajjasani virajjasani dismissed their stale review August 28, 2020 06:55

Until test is fixed

@rda3mon
Copy link
Contributor Author

rda3mon commented Sep 5, 2020

@virajjasani I spent a good day and half. Couldn't solve this issue. Tagging origin author to help me on this @PierreZ and have written a detailed email.

Brief detail:
On correcting this line if (fromRegion >= 0 && toRegion >= 0) { from if (fromRegion > 0 && toRegion > 0) {. TestStochasticLoadBalancerHeterogeneousCost tests broke down.

@virajjasani
Copy link
Contributor

virajjasani commented Sep 6, 2020

Thanks @rda3mon for all your efforts. IMHO, test might require modification in this case. However, to cross verify, you can initiate a mail thread on dev@hbase asking if anyone with better understanding of TestStochasticLoadBalancerHeterogeneousCost can confirm whether your source code change is directly proportional to test change and also if it is fine increasing ALLOWED_WINDOW or any better approach should help.

@rda3mon
Copy link
Contributor Author

rda3mon commented Sep 7, 2020

@virajjasani Original author @PierreZ will be able to look at this post 14th Sep as he doesn't have access to laptop until then.. So, will wait till then.

@rda3mon
Copy link
Contributor Author

rda3mon commented Sep 7, 2020

@virajjasani Increase in ALLOWED_WINDOW will not be of much help. Because, the deviation isn't small. Expected 50 * 0.4 = 20. Actual is 49.

@rda3mon
Copy link
Contributor Author

rda3mon commented Jan 15, 2021

@virajjasani I am closing this as I couldn't fix the test cases and it was very tricky to narrow down the issue.

@Apache9
Copy link
Contributor

Apache9 commented Mar 15, 2021

FIxed by #3024

@Apache9 Apache9 closed this Mar 15, 2021
@rda3mon rda3mon deleted the HBASE-24931-candidate-generator-getAction-ignore-zeroth-index branch April 10, 2021 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@virajjasani virajjasani virajjasani left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rda3mon @Apache-HBase @virajjasani @Apache9