Uh oh!
There was an error while loading. Please reload this page.
HBASE-25739 TableSkewCostFunction need to use aggregated deviation - #3415
Merged
Conversation
Apache-HBase
commented
Jun 23, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 23, 2021
🎊 +1 overall
This message was automatically generated. |
clarax
commented
Jun 23, 2021
ContributorAuthor
Enabling runMaxStep and increasing max run time for TestStochasticBalancerLargeCluster takes care of the flaky tests. I removed the increase for max run time for TestStochasticBalancerBalanceCluster since it is not really needed and increase total rest run time. |
Apache9
reviewed
Jun 24, 2021
| } | ||
| numTables = tables.size(); | ||
| LOG.info("number of tables = {}", numTables); |
Contributor
There was a problem hiding this comment.
Is info the suitable level here? Or debug?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| /** | ||
| * Return the min skew of distribution | ||
| */ | ||
| public static double getMinSkew(double total, double numServers) { |
Member
There was a problem hiding this comment.
Is total the "total number of regions in the cluster"?
Contributor
There was a problem hiding this comment.
nit: one way of addressing the nick comment would be to add javadoc for total that said what it was.....
| /** | ||
| * Return the min skew of distribution | ||
| */ | ||
| public static double getMinSkew(double total, double numServers) { |
Member
There was a problem hiding this comment.
Why are the input arguments double? Can there be a fractional amount of either of these quantities?
ContributorAuthor
There was a problem hiding this comment.
It is to convert the input from integer to double for computation in the function.
| double mean = total / numServers; | ||
| // It's possible that there aren't enough regions to go around | ||
| double min; | ||
| if (numServers > total) { |
ContributorAuthor
There was a problem hiding this comment.
This is the case when we have more nodes than regions, we will have nodes without regions and it is balanced.
| tempFunctionCosts = new double[costFunctions.size()]; | ||
| LOG.info("Loaded config; maxSteps=" + maxSteps + ", stepsPerRegion=" + stepsPerRegion + | ||
| LOG.info("Loaded config; maxSteps=" + maxSteps + " ,runMaxSteps=" + runMaxSteps, |
| - meanRegionsPerTable[tableIdx]); | ||
| } | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
clarax
commented
Jun 24, 2021
ContributorAuthor
"values like minCostNeedBalance below are at 0.00 precision, so I think we should have an epsilon of at least 0.000 precision." Actually not really. this is aggregated deviation before scaling or divided by multiplier so it is at the precision of close to 1 or 0.1. But it is good to go lower to 0.001. |
Apache-HBase
commented
Jun 25, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 25, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 25, 2021
🎊 +1 overall
This message was automatically generated. |
clarax
commented
Jun 25, 2021
ContributorAuthor
Apache9
approved these changes
Jul 3, 2021
Apache9
commented
Jul 3, 2021
Contributor
Please fix the checkstyle issue before merging. Thanks. |
Apache-HBase
commented
Jul 11, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 11, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 11, 2021
🎊 +1 overall
This message was automatically generated. |
clarax
commented
Jul 12, 2021
ContributorAuthor
I don't understand why there is the Javac complaint. I didn't even touch the file. |
Apache9
commented
Jul 12, 2021
Contributor
The error prone output is not very stable, so it is just a warning, not a blocker, unless error prone fails the compliation. Just go ahead. |
saintstack
approved these changes
Jul 12, 2021
saintstack
left a comment
Contributor
There was a problem hiding this comment.
LGTM.
A few little nits in below for next time.
| } | ||
| numTables = tables.size(); | ||
| LOG.debug("number of tables = {}", numTables); |
Contributor
There was a problem hiding this comment.
nit: for next time, we capitalize log messages and if you look at other logs, there is no space around '=' when used in log messages.
| } | ||
| for (int tableIdx = 0; tableIdx < aNumRegionsPerServerPerTable.length; tableIdx++) { | ||
| regionSkewByTable[tableIdx] += Math.abs(aNumRegionsPerServerPerTable[tableIdx] | ||
| - meanRegionsPerTable[tableIdx]); |
Contributor
There was a problem hiding this comment.
nit: style. In the code base, we usually have operator on the end of the line rather than the start as it is here... When on the end of the line, the line looks to be 'hanging' so dev will continue reading... With this style, the dev might miss the continuation. Just style.
| Math.abs(numRegionsPerServerPerTable[newServer][tableIndex] | ||
| - meanRegionsPerTable[tableIndex]) | ||
| - Math.abs(numRegionsPerServerPerTable[newServer][tableIndex] - 1 | ||
| - meanRegionsPerTable[tableIndex]); |
Contributor
There was a problem hiding this comment.
nit: hard to read. You might do local variables just to make it easier in the future rather than this long line
| /** | ||
| * Return the min skew of distribution | ||
| */ | ||
| public static double getMinSkew(double total, double numServers) { |
Contributor
There was a problem hiding this comment.
nit: one way of addressing the nick comment would be to add javadoc for total that said what it was.....
| min = (numHigh * (Math.ceil(mean) - mean)) + (numLow * (mean - Math.floor(mean))); | ||
| } | ||
| min = Math.max(0, min); |
ContributorAuthor
There was a problem hiding this comment.
applied later by prior code change.
Apache-HBase
commented
Jul 12, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 12, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 12, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 13, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 13, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 13, 2021
🎊 +1 overall
This message was automatically generated. |
saintstack
commented
Jul 13, 2021
Contributor
Merged. All unit tests passed. The complaint was from cleanup post-build messaging H2 machine. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-apply the patch that was merged and reverted for flaky test because of refactoring afterwards.
The flaky tests was actually caused by two bugs: incorrect implementation of TableSkewCostfunction in the old codes and the tests were bent to pass.
There is another bug in the original tableSkew cost function for aggregation of the cost per table:
If we have 10 regions, one per table, evenly distributed on 10 nodes, the cost is scale to 1.0.
The more tables we have, the closer the value will be to 1.0. The cost function becomes useless.
All the balancer tests were set up with large numbers of tables with minimal regions per table. This artificially inflates the total cost and trigger balancer runs. Because of the fix, the default 0.05 minCostNeedBalance will not quite work. As a gap-stopper before I check in auto-tuning threshold, I reduce the default value to 0.025 so to keep the user experience consistent. We in production use an even lower value for a very large cluster.