HBASE-22618 Provide a way to have Heterogeneous deployment #439
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
reviewed
Aug 2, 2019
| try { | ||
| klass = (Class<? extends CostFunction>) Class.forName(c); | ||
| } catch (ClassNotFoundException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Should log a WARN, as e.printStackTrace goes to the stdo.
wchevreuil
reviewed
Aug 2, 2019
| @@ -457,6 +450,13 @@ public void testLosingRs() throws Exception { | |||
| assertNull(plans); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
How about test that, upon StochasticLoadBalancer.balanceCluster() call, verify that DummyCostFunctions.cost() get indeed called? That would validate custom cost functions are indeed used.
There was a problem hiding this comment.
I was thinking about adding tests with the real cost function I want to implement, is that OK for you?
There was a problem hiding this comment.
Maybe we should keep this jira concerned only with the implementation to allow additional cost functions to be added. Then you could propose another jira with the real implementation of your cost functions, which then would also define those functions specific tests. What would you say?
|
Thanks for the contribution @PierreZ ! I had made some small nits remarks on the PR. Please address the reported checkstyle issues added by these changes. Reported UT failures should be unrelated, verified those locally and those passed. |
|
Thanks for your comments @wchevreuil! The checkstyles issues should be fixed now. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
There's still a minor checkstyle issue in TestStochasticLoadBalancer. You may want to take a look at this ref guide section for tips on how to setup eclipse/intelij for hbase code format. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Signed-off-by: Pierre Zemb <contact@pierrezemb.fr>
|
💔 -1 overall
This message was automatically generated. |
|
I can't understand the checkstyle problem with the import order, here's the report: But lines 47/48 does not correspond to the specified lines in the report. |
|
Hum, I think force-pushes confuse github PRs. Let me run checkstyles locally to make sure it's ok. Next time, just push single commits to the PR, then we can rebase/squash all of them prior to merging. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
reviewed
Aug 10, 2019
| @@ -53,6 +47,10 @@ | |||
| import org.junit.Test; | |||
| import org.junit.experimental.categories.Category; | |||
|
|
|||
| import static org.junit.Assert.*; | |||
There was a problem hiding this comment.
Am getting the following checkstyle issues, when running checkstyle locally:
| Error | imports | AvoidStarImport | Using the '.' form of import should be avoided - org.junit.Assert.. | 50 |
|---|---|---|---|---|
| Error | imports | ImportOrder | Wrong order for 'org.junit.Assert.*' import. | 50 |
There was a problem hiding this comment.
I'm sorry for the trouble, I must admit that I was trusting the import style of my IDE 😄
Will fix it in another commit that will be checked locally this time
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Hi! It seems I have different failed junit tests at each run, should I be worried? |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
The intermittent failures are not really related. Latest changes look good. Let me rebase it locally, get a patch and submit it in the jira for a last round of pre-commit tests. |
|
💔 -1 overall
This message was automatically generated. |
Thank you @wchevreuil ! I'm wondering what is the process to push the patch on the 1.X branch? |
|
💔 -1 overall
This message was automatically generated. |
|
Checkstyle issues all fixed. Current PR looks good. As mentioned previously, rebased it locally and will attach it as a patch in the jira for another run of pre-commit tests. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Hi!
This is the PR regarding HBASE-22618 . The goal of the PR is to allow Heterogeneous HBase clusters.
We would like to allow users to load custom cost functions that can implement newer ways of balance regions.
This first commit is providing to possibility load them, and a second commit on this branch will add an working example of a custom cost function. This custom function will allow users to balance regions according to an allowed number of regions per RS.
Before implementing the cost function, I want to validate first the dynamic load 😃