Uh oh!
There was an error while loading. Please reload this page.
HBASE-23647: Make MasterRegistry the default impl. - #1039
Merged
apurtell merged 2 commits intoJan 27, 2020
Merged
Conversation
bharathv
commented
Jan 14, 2020
bharathv
left a comment
ContributorAuthor
There was a problem hiding this comment.
Do not merge this yet. Lets see what breaks in the PR jenkins run.
Apache-HBase
commented
Jan 14, 2020
💔 -1 overall
This message was automatically generated. |
bharathv
commented
Jan 15, 2020
ContributorAuthor
A bunch of tests failed with "Connection Refused" errors on the master ports serving the RPCs. Let me dig into the logs and update here. |
bharathv
commented
Jan 17, 2020
bharathv
left a comment
ContributorAuthor
There was a problem hiding this comment.
Cleans up a subset of test failures. Pushing it out to reduce the noise in the test report. Let's see how it looks like after this change.
Majority of the remaining failures have something to do with the killing of the only active master and that doesn't go well with master based registry implementation. I'm still thinking on how best to fix those cases while still retaining the test coverage.
Apache-HBase
commented
Jan 17, 2020
💔 -1 overall
This message was automatically generated. |
bharathv
commented
Jan 20, 2020
bharathv
left a comment
ContributorAuthor
There was a problem hiding this comment.
Latest patch fixes most tests except a few replication tests that I'm still debugging. Meanwhile, want to do a quick run to make sure nothing else is broken.
Apache-HBase
commented
Jan 20, 2020
💔 -1 overall
This message was automatically generated. |
bharathvforce-pushed
the
HBASE-23647
branch
2 times, most recently
from
January 21, 2020 00:25
6ec93fe to
a9cf71dComparebharathv
commented
Jan 21, 2020
bharathv
left a comment
ContributorAuthor
There was a problem hiding this comment.
I think the last pushed patchset fixes all the test issues (atleast for me locally). Lets see how it does in a jenkins run. If its green, the patch is ready for review.
Apache-HBase
commented
Jan 21, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 21, 2020
💔 -1 overall
This message was automatically generated. |
apurtell
commented
Jan 22, 2020
Contributor
So the only test failure now is TestFromClientSide? Can this be excluded as not related or a flake? |
bharathv
commented
Jan 22, 2020
ContributorAuthor
Reposting from the jira comment: @apurtell TestFromClientSide was known to be flaky (even before this patch). So I'm fairly certain it has nothing to do with the current jira. One thing @ndimiduk and I noticed here is that switching to JUnit 4.13 (HBASE-23664) has exacerbated the problem. Based on some initial debugging it looks like it has something to do with leaking FileSystem/DFSClient objects in the HBase code and restarting the minicluster in the same JUnit test runner JVM (what TestFromClientSide does) causes a leak of hdfs lease renewer threads. Some how Junit 4.12 was masking the problem. |
apurtell
approved these changes
Jan 22, 2020
apurtell
left a comment
Contributor
There was a problem hiding this comment.
A couple of suggestions.
Lots of nice test improvements in here too.
| // Without the right registry, the above configs are useless. Also, we don't use setClass() | ||
| // here because the ConnectionRegistry* classes are not resolvable from this module. | ||
| // This will be broken if ZkConnectionRegistry class gets renamed or moved. Is there a better | ||
| // way? |
Contributor
There was a problem hiding this comment.
You could use findClass and if there's an exception fall through to alternate or recovery code. Anyway, agreed, a reference to a class constant is not called for here.
ContributorAuthor
There was a problem hiding this comment.
fall through to alternate or recovery code
There is no alternate or recovery from that point, no? The same error is propagated while creating the registry instance, so I guess we don't need to do it again here I think.
Contributor
There was a problem hiding this comment.
This code is going waste away. If user chooses zk registry, this code applies?
ContributorAuthor
There was a problem hiding this comment.
Agree. It is ok to leave as-is since it is a no-op?
Member
There was a problem hiding this comment.
I'm finding this method used by SyncTable, TableMapReduceUtil, TableOutputFormat, VerifyReplication, ExportSnapshot ... Per earlier discussion, replication will continue to use the ZK registry... what about the general MapReduce/Spark/Flink use-cases?
ContributorAuthor
There was a problem hiding this comment.
Hmm. Good point.
TableMapReduceUtil: This happens only in initCredentialsForCluster() or if hbase.mapred.output.quorum is specified. Basically it only happens for a "peer" cluster. (same for TableOutputFormat, unless QUORUM_ADDRESS for a target is specified this doesn't happen). Overall, I think all MR jobs running on a single cluster will use master registry. I think that answers the MR/Spark/Flink usecases.
Now coming to MR jobs spanning multiple clusters (source and target) Ex: VerifyReplication / ExportSnapshot/SyncTable/TableOutputFormat etc
I think these need to be rewritten with new config params like hbase.[source|target].master.addrs for clients to pass the addresses so that they can use master registry.
What do you think? Should we rewrite them with new configs or maintain compatibility and keep using zkregistry?
Member
There was a problem hiding this comment.
Overall, I think all MR jobs running on a single cluster will use master registry. I think that answers the MR/Spark/Flink usecases.
Sounds good.
I think these need to be rewritten with new config params like hbase.[source|target].master.addrs for clients to pass the addresses so that they can use master registry.
I think it's best to reduce the ZK-exposed surface area as much as possible. This seems a reasonable solution to me.
| // - Decouples RS and master life cycles. For example, if all the masters are down, region | ||
| // servers can abort at the same time, because the internal connection is master dependent and | ||
| // fails.This is an operational nightmare. Using the ZK based registry means that the region | ||
| // servers are now on the look out for new masters, if they are spun up. |
Contributor
There was a problem hiding this comment.
The tense here is confusing.
"Using the ZK based registry means the region servers continue to be independent of master availability..." ?
| // - We need to retain ZKConnectionRegistry for replication use anyway, so we just extend it for | ||
| // other internal connections too. | ||
| conf.set(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY, | ||
| "org.apache.hadoop.hbase.client.ZKConnectionRegistry"); |
Contributor
There was a problem hiding this comment.
Ok, but these class names can be constant strings at least.
| // ZK registry is so slow that by then the server manager is init'ed thus masking the problem. | ||
| // For now, I'm putting a sleep here to workaround the issue, I think the fix for it is a little | ||
| // delicate and needs to be done separately. | ||
| Thread.sleep(5000); |
Contributor
There was a problem hiding this comment.
Is it possible to use Waiter#waitFor to wait on some condition variable or predicate? Hard coded sleeps in tests tend to fall over on Apache Jenkins because of load issues ie the sleeps can never be long enough...
saintstack
left a comment
Contributor
There was a problem hiding this comment.
Looks great.
One question probably answered over in core patch, is there any writeup on how things will run from here on out when we have Master Registry in place? If no Masters, clients will have no place to go. What is the failure like? Spewing can't connect? If Master comes up, all smooths out? Etc. Thanks.
| // Without the right registry, the above configs are useless. Also, we don't use setClass() | ||
| // here because the ConnectionRegistry* classes are not resolvable from this module. | ||
| // This will be broken if ZkConnectionRegistry class gets renamed or moved. Is there a better | ||
| // way? |
Contributor
There was a problem hiding this comment.
This code is going waste away. If user chooses zk registry, this code applies?
Uh oh!
There was an error while loading. Please reload this page.
| List<String> masterHostPorts = new ArrayList<>(); | ||
| getMasters().forEach(masterThread -> | ||
| masterHostPorts.add(masterThread.getMaster().getServerName().getAddress().toString())); | ||
| conf.set(HConstants.MASTER_ADDRS_KEY, String.join(",", masterHostPorts)); |
Contributor
There was a problem hiding this comment.
I could be obnoxious here but won't. Better to do as you have done here and just move the code.... not change it.
ContributorAuthor
There was a problem hiding this comment.
Ah I rewrote this multiple times to get it working and in that process called it from multiple places, hence separated it out into a method. Doesn't make sense anymore like you pointed out. Moved it to the new place instead.
Uh oh!
There was an error while loading. Please reload this page.
| // - Decouples RS and master life cycles. RegionServers can continue be up independent of | ||
| // masters' availability. | ||
| // - Configuration management for region servers (cluster internal) is much simpler when adding | ||
| // new masters etc. |
Contributor
There was a problem hiding this comment.
I don't understand this one.
ContributorAuthor
There was a problem hiding this comment.
So if one is adding new masters to the cluster (or taking down an existing master), they will have to update the region servers config by doing a rolling restart (unless there is some dynamic way of updating it). This operational overhead is avoided since the regionservers figure it out from ZK. They just have to update it on the clients.
ContributorAuthor
There was a problem hiding this comment.
(clarified it in the comment, let me know if it is still unclear)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| TEST_UTIL.createRootDir(); | ||
| final LocalHBaseCluster cluster = | ||
| new LocalHBaseCluster(conf, NUM_MASTERS, NUM_RS, HMaster.class, | ||
| new LocalHBaseCluster(conf, NUM_MASTERS, 0, NUM_RS, HMaster.class, |
Contributor
There was a problem hiding this comment.
We seem to do '0' as AlwaysMasters. Should we do override that defaults zero? Just to keep the AlwaysMasters out of view when not needed.
Apache-HBase
commented
Jan 23, 2020
💔 -1 overall
This message was automatically generated. |
bharathv
commented
Jan 23, 2020
bharathv
left a comment
ContributorAuthor
There was a problem hiding this comment.
is there any writeup on how things will run from here on out when we have Master Registry in place?
Plan to document as a part of HBASE-23331.
If no Masters, clients will have no place to go. What is the failure like? Spewing can't connect? If Master comes up, all smooths out? Etc. Thanks.
Ya, as long as the master ports (atleast one of the configured ones) didn't change, connection should be back up. Until then spewing of stack traces
| // Without the right registry, the above configs are useless. Also, we don't use setClass() | ||
| // here because the ConnectionRegistry* classes are not resolvable from this module. | ||
| // This will be broken if ZkConnectionRegistry class gets renamed or moved. Is there a better | ||
| // way? |
ContributorAuthor
There was a problem hiding this comment.
Agree. It is ok to leave as-is since it is a no-op?
Uh oh!
There was an error while loading. Please reload this page.
| List<String> masterHostPorts = new ArrayList<>(); | ||
| getMasters().forEach(masterThread -> | ||
| masterHostPorts.add(masterThread.getMaster().getServerName().getAddress().toString())); | ||
| conf.set(HConstants.MASTER_ADDRS_KEY, String.join(",", masterHostPorts)); |
ContributorAuthor
There was a problem hiding this comment.
Ah I rewrote this multiple times to get it working and in that process called it from multiple places, hence separated it out into a method. Doesn't make sense anymore like you pointed out. Moved it to the new place instead.
Uh oh!
There was an error while loading. Please reload this page.
| // - Decouples RS and master life cycles. RegionServers can continue be up independent of | ||
| // masters' availability. | ||
| // - Configuration management for region servers (cluster internal) is much simpler when adding | ||
| // new masters etc. |
ContributorAuthor
There was a problem hiding this comment.
So if one is adding new masters to the cluster (or taking down an existing master), they will have to update the region servers config by doing a rolling restart (unless there is some dynamic way of updating it). This operational overhead is avoided since the regionservers figure it out from ZK. They just have to update it on the clients.
Uh oh!
There was an error while loading. Please reload this page.
| TEST_UTIL.startMiniCluster(3); | ||
| REGISTRY = ConnectionRegistryFactory.getRegistry(TEST_UTIL.getConfiguration()); | ||
| RegionReplicaTestHelper.waitUntilAllMetaReplicasHavingRegionLocation( | ||
| RegionReplicaTestHelper.waitUntilAllMetaReplicasAreReady(TEST_UTIL, |
ContributorAuthor
There was a problem hiding this comment.
good catch, you are right. For some reason I thought there are some callers that pass some custom config but I was wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| TEST_UTIL.createRootDir(); | ||
| final LocalHBaseCluster cluster = | ||
| new LocalHBaseCluster(conf, NUM_MASTERS, NUM_RS, HMaster.class, | ||
| new LocalHBaseCluster(conf, NUM_MASTERS, 0, NUM_RS, HMaster.class, |
Apache-HBase
commented
Jan 23, 2020
💔 -1 overall
This message was automatically generated. |
saintstack
approved these changes
Jan 23, 2020
saintstack
left a comment
Contributor
There was a problem hiding this comment.
LGTM
....and I felt this is redundant (tests the same thing) so removed. You prefer to retain it?
We need more of this sort of judgement -- purging redundant tests.
They just have to update it on the clients.
Hmm. If a client has three Masters, and the operators remove one, what then? Operator has to update client configs to purge the removed Master?
I suppose we have same issue w/ zk ensemble list.
Agree, it is a test-only construct. Didn't leak into the server module. Anything else should be done?
No. Just hide it in tests as much as you can. Be explicit it is a test-only item (which I think you do...).
Agree. It is ok to leave as-is since it is a no-op?
Yes. Ok as is.
bharathv
commented
Jan 23, 2020
ContributorAuthor
Ya, thats the same problem, agreed. |
asfgitforce-pushed
the
HBASE-18095/client-locate-meta-no-zookeeper
branch
from
January 24, 2020 17:45
d9bb034 to
62da419Comparebharathv
commented
Jan 24, 2020
bharathv
left a comment
ContributorAuthor
There was a problem hiding this comment.
Rebased and squashed after a force push to the feature branch. Was a clean one.
ndimiduk
approved these changes
Jan 24, 2020
ndimiduk
left a comment
Member
There was a problem hiding this comment.
I have a couple small change requests, but overall this is looking good.
| conf.set(HConstants.ZOOKEEPER_QUORUM, zkClusterKey.getQuorumString()); | ||
| conf.setInt(HConstants.ZOOKEEPER_CLIENT_PORT, zkClusterKey.getClientPort()); | ||
| conf.set(HConstants.ZOOKEEPER_ZNODE_PARENT, zkClusterKey.getZnodeParent()); | ||
| // Without the right registry, the above configs are useless. Also, we don't use setClass() |
Member
There was a problem hiding this comment.
Is there a check-then-fail that can be done to assert that the method invocation has significance? Maybe log a warning saying zk configs are being applied to a master registry?
ContributorAuthor
There was a problem hiding this comment.
Added the logging. Don't think it should be warn though.
| // Without the right registry, the above configs are useless. Also, we don't use setClass() | ||
| // here because the ConnectionRegistry* classes are not resolvable from this module. | ||
| // This will be broken if ZkConnectionRegistry class gets renamed or moved. Is there a better | ||
| // way? |
Member
There was a problem hiding this comment.
I'm finding this method used by SyncTable, TableMapReduceUtil, TableOutputFormat, VerifyReplication, ExportSnapshot ... Per earlier discussion, replication will continue to use the ZK registry... what about the general MapReduce/Spark/Flink use-cases?
| final int noRegionServers, final Class<? extends HMaster> masterClass, | ||
| final Class<? extends HRegionServer> regionServerClass) | ||
| throws IOException { | ||
| final int noAlwaysStandByMasters, final int noRegionServers, |
Member
There was a problem hiding this comment.
Ugh. The final constructor args are Configuration, int, int, int, Class, Class. It's time for a builder with named arguments.
ContributorAuthor
There was a problem hiding this comment.
Ya, too long and confusing now.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| // the master. The reason it was not happening earlier is because the connection creation with | ||
| // ZK registry is so slow that by then the server manager is init'ed thus masking the problem. | ||
| // For now, I'm putting a wait() here to workaround the issue, I think the fix for it is a | ||
| // little delicate and needs to be done separately. |
Member
There was a problem hiding this comment.
What's the fix you have in mind? Reading through your comment, I thought of a test-only coprocessor that installs a latch that can be waited upon by the test.
ContributorAuthor
There was a problem hiding this comment.
I was thinking about fixing the actual problem rather than the test. If a shutdown happens before starting the server manager, the latter takes note of that during init and shuts down automatically (or something along the lines). My fix is in the master bootstrap.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Jan 25, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 26, 2020
💔 -1 overall
This message was automatically generated. |
asfgit pushed a commit
that referenced
this pull request
Jan 29, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit
that referenced
this pull request
Jan 29, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit
that referenced
this pull request
Jan 30, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 3, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 4, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 5, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit
that referenced
this pull request
Feb 5, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit
that referenced
this pull request
Feb 9, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit
that referenced
this pull request
Feb 11, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit
that referenced
this pull request
Feb 13, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit
to bharathv/hbase
that referenced
this pull request
Feb 14, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit
to bharathv/hbase
that referenced
this pull request
Feb 17, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit
that referenced
this pull request
Feb 18, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit
to bharathv/hbase
that referenced
this pull request
Feb 20, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit
that referenced
this pull request
Feb 20, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit
to bharathv/hbase
that referenced
this pull request
Feb 23, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> (cherry picked from commit 229b8aa)
bharathv added a commit
to bharathv/hbase
that referenced
this pull request
Feb 24, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> (cherry picked from commit 229b8aa)
bharathv added a commit
to bharathv/hbase
that referenced
this pull request
Feb 25, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> (cherry picked from commit 229b8aa)
bharathv added a commit
to bharathv/hbase
that referenced
this pull request
Feb 26, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> (cherry picked from commit 229b8aa)
bharathv added a commit
that referenced
this pull request
Feb 27, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> (cherry picked from commit 229b8aa)
thangTang pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
thangTang pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
Signed-off-by: Stack <stack@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org>
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.
No description provided.