Uh oh!
There was an error while loading. Please reload this page.
HBASE-26532 Replication could choose the same named group if it is exist in the target cluster - #3911
Closed
thangTang wants to merge 1 commit into
Closed
HBASE-26532 Replication could choose the same named group if it is exist in the target cluster#3911thangTang wants to merge 1 commit into
thangTang wants to merge 1 commit into
Conversation
Apache-HBase
commented
Dec 2, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 2, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 2, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 2, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 2, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 2, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 2, 2021
💔 -1 overall
This message was automatically generated. |
…ist in the target cluster
Apache-HBase
commented
Dec 2, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 2, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 2, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 2, 2021
🎊 +1 overall
This message was automatically generated. |
| // Ratio of total number of potential peer region servers to be used | ||
| private float ratio; | ||
| private float groupRatio; |
Contributor
There was a problem hiding this comment.
here can add some descriptions/comments as well
Contributor
There was a problem hiding this comment.
Yes, please add some explaination.
| } | ||
| }; | ||
| public boolean getIsGroup() { |
| if (children == null) { | ||
| return Collections.emptyList(); | ||
| } | ||
| StringBuffer sb = new StringBuffer(); |
| } | ||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug( | ||
| "Find " + addresses.size() + " child znodes from target cluster zk. " + sb.toString()); |
Contributor
There was a problem hiding this comment.
Please use parameterized logging, and elsewhere.
| LOG.debug("Use replication rsgroup choose policy..."); | ||
| } | ||
| Map<String, String> serverNameHostPortMapping = new HashMap<>(); | ||
| for (String serverName : children) { |
Contributor
There was a problem hiding this comment.
can reuse parseServerNameFromList(children) ? then the followings can just user ServerName#getHost and ServerName#getPort.
| getGroupServerListFromTargetZkCluster(groupName, zkw, serverNameHostPortMapping); | ||
| if (serverList.size() > 0) { | ||
| // if target cluster open group balancer, serverList must has server(s) | ||
| LOG.debug("group list > 0"); |
| try { | ||
| rsGroupInfo = getRSGroupInfoOfServer(conn.toConnection(), hostServerName.getAddress()); | ||
| }catch (IOException e) { | ||
| e.printStackTrace(); |
Contributor
There was a problem hiding this comment.
still neede.printStackTrace();?
| } | ||
| else { | ||
| // if not, choose sinkers from all regionservers | ||
| LOG.debug("target group list <= 0"); |
Contributor
There was a problem hiding this comment.
ditto. Please add check LOG.isDebugEnabled() {} and elsewhere
| LOG.debug("groupInfos == null"); | ||
| } | ||
| return Collections.emptyList(); | ||
| }else{ |
Contributor
There was a problem hiding this comment.
nit. spaces between else {} are needed
| int numSinks = (int) Math.ceil(slaveAddresses.size() * actualRatio); | ||
| this.sinkServers = slaveAddresses.subList(0, numSinks); | ||
| StringBuffer sb = new StringBuffer(); |
Reidddddd
commented
Dec 3, 2021
Contributor
ping @Apache9, you probably would want to take a look at this one? |
Apache9
reviewed
Dec 4, 2021
Apache9
left a comment
Contributor
There was a problem hiding this comment.
Seems there are still some legacy code, which is for the old rs group implementation. Better provide a simple design doc first, we can discuss the approach first, and then you can modify the PR here.
Thanks~
| */ | ||
| @InterfaceAudience.Private | ||
| public interface ReplicationService { | ||
Contributor
There was a problem hiding this comment.
Please avoid touching unnecessary file.
| // Ratio of total number of potential peer region servers to be used | ||
| private float ratio; | ||
| private float groupRatio; |
Contributor
There was a problem hiding this comment.
Yes, please add some explaination.
| private List<ServerName> sinkServers = new ArrayList<>(0); | ||
| private static ThreadLocal<AtomicBoolean> threadLocal = new ThreadLocal<AtomicBoolean>() { |
Contributor
There was a problem hiding this comment.
We need to use AtomicBoolean for a thread local variable?
| Configuration conf = HBaseConfiguration.create(); | ||
| /** if use other balancer, return all regionservers */ | ||
| if (!conf.get(HConstants.HBASE_MASTER_LOADBALANCER_CLASS) |
Contributor
There was a problem hiding this comment.
On master this is not the case now. The balancer will always be a RSGroupBasedLoadBalancer, if we do not enable rs group feature, DisabledRSGroupInfoManager will be used and there will be only one group, which will act as there is no rs group. So I think here we should check for RSGroupUtil.isRSGroupEnabled.
Contributor
There was a problem hiding this comment.
And another problem is do we need to check the configuration here? I think it is the version at the source cluster, but we need to check the target cluster?
| return addresses; | ||
| } | ||
| protected List<ServerName> getGroupServerListFromTargetZkCluster(String groupName, |
Contributor
There was a problem hiding this comment.
On master branch we do not use zk for storing the rs group any more...
thangTang
commented
Dec 5, 2021
ContributorAuthor
Yes, actually i did this on 1.x... |
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
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.
https://issues.apache.org/jira/browse/HBASE-26532