HBASE-25852 Move all the intialization work of LoadBalancer implement… #3248
Conversation
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
|
💔 -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. |
…ation to initialize method
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
approved these changes
May 10, 2021
Contributor
saintstack
left a comment
saintstack
left a comment
There was a problem hiding this comment.
LG.
Good cleanup.
Nit questions/Comments
| * Set the current cluster status. This allows a LoadBalancer to map host name to a server | ||
| */ | ||
| void setClusterMetrics(ClusterMetrics st); | ||
| void updateClusterMetrics(ClusterMetrics metrics); |
Contributor
There was a problem hiding this comment.
We change the method name because the method is now being used differently? Before we called 'set' once? But now we 'update' continuously during operation? Or was it just always incorrectly named?
Contributor
Author
There was a problem hiding this comment.
We have a background chore called ClusterStatusChore to call this method periodically...
| * <p/> | ||
| * Since 3.0.0, all the balancers will be wrapped inside a {@code RSGroupBasedLoadBalancer}, it will | ||
| * be in charge of the synchronization of balancing and configuration changing, so we do not need to | ||
| * synchronized by ourselves. |
Contributor
There was a problem hiding this comment.
Interesting. Wondering what the rationale for this (Perhaps it later in this patch)
Contributor
Author
There was a problem hiding this comment.
It is because a previous work, where we move rsgroup feature to core hbase instead of using coprocessor.
See HBASE-22514.
| .generate(cluster); | ||
| } | ||
|
|
||
| @RestrictedApi(explanation = "Should only be called in tests", link = "", |
Contributor
There was a problem hiding this comment.
Instead of @VisibleForTests? (I like this annotation)
Contributor
Author
There was a problem hiding this comment.
Yes, @apurtell proposed to remove the VisibleForTesting annotation and the javadoc for VisibleForTestinng also tells that you should not use this annotation anymore and suggests you to use RestrictedApi.
/**
* Annotates a program element that exists, or is more widely visible than otherwise necessary, only
* for use in test code.
*
* <p><b>Do not use this interface</b> for public or protected declarations: it is a fig leaf for
* bad design, and it does not prevent anyone from using the declaration---and experience has shown
* that they will. If the method breaks the encapsulation of its class, then its internal
* representation will be hard to change. Instead, use <a
* href="http://errorprone.info/bugpattern/RestrictedApiChecker">RestrictedApiChecker</a>, which
* enforces fine-grained visibility policies.
*
* @author Johannes Henkel
*/
@GwtCompatible
public @interface VisibleForTesting {
}
We will enable error prone when building the project in the general check state in pre commit, so if you break the restriction, there will be a compile error, which is much more useful than VisibleForTesting.
Apache9
added a commit
to Apache9/hbase
that referenced
this pull request
May 12, 2021
…ation to initialize method (apache#3248) Signed-off-by: Michael Stack <[email protected]> Signed-off-by: Yulin Niu <[email protected]>
Apache9
added a commit
that referenced
this pull request
May 13, 2021
…ation to initialize method (#3248) (#3258) Signed-off-by: Michael Stack <[email protected]> Signed-off-by: Yulin Niu <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
…ation to initialize method