Uh oh!
There was an error while loading. Please reload this page.
HBASE-24765: Dynamic master discovery - #2130
Merged
Merged
Conversation
Apache-HBase
commented
Jul 23, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 23, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 24, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 24, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 24, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 24, 2020
🎊 +1 overall
This message was automatically generated. |
virajjasani
left a comment
Contributor
There was a problem hiding this comment.
Nice patch, left some comments. PR should point to splittable meta feature branch?
| .setServerName(ProtobufUtil.toServerName(name)).setIsActive(true).build())); | ||
| // Backup masters | ||
| try { | ||
| // TODO: Cache the backup masters to avoid a ZK RPC for each getMasters() call. |
Contributor
There was a problem hiding this comment.
We are planning to have a cache with ZKWatcher for backupMasters ZNode right? I believe as of now, we don't subscribe for any event.
| }; | ||
| masterAddrRefresherThread = Threads.newDaemonThreadFactory( | ||
| "MasterRegistry refresh end-points").newThread(masterEndPointRefresher); | ||
| masterAddrRefresherThread.start(); |
Contributor
There was a problem hiding this comment.
Don't want to use SingleThreadExecutor.submit()?
ContributorAuthor
There was a problem hiding this comment.
Ok switched. I didn't want to have extra layers on top of a simple thread, but I guess a pool is more readable.
Comment on lines
+163
to
+164
| masterAddrRefresherThread = Threads.newDaemonThreadFactory( | ||
| "MasterRegistry refresh end-points").newThread(masterEndPointRefresher); |
Contributor
There was a problem hiding this comment.
Can you please use guava library's ThreadFactoryBuilder? So far the consensus on HBASE-24750 is to get rid of our internally maintained ThreadFactory :)
ContributorAuthor
There was a problem hiding this comment.
Ok switched. I think we should use our internal one, but fine.
| } | ||
| }; | ||
| masterAddrRefresherThread = Threads.newDaemonThreadFactory( | ||
| "MasterRegistry refresh end-points").newThread(masterEndPointRefresher); |
Contributor
There was a problem hiding this comment.
nit: avoid space in Thread prefix name?
| registry.getParsedMasterServers().size() == 2); | ||
| final Set<ServerName> newMasters2 = registry.getParsedMasterServers(); | ||
| assertEquals(2, newMasters2.size()); | ||
| assertFalse(newMasters2.contains(activeMaster)); |
Contributor
There was a problem hiding this comment.
newMasters2.contains(activeMaster.getServerName())
Comment on lines
+156
to
+159
| TEST_UTIL.waitFor(5000, | ||
| (Waiter.Predicate<Exception>) () -> !registry.getParsedMasterServers().equals(masters)); |
Contributor
There was a problem hiding this comment.
nit: upto you if you want to use ExplainingPredicate to throw Exception with specific message
ContributorAuthor
There was a problem hiding this comment.
Skipped it because it bloats up the code, we need to expand the lambda. I think the intent there is pretty clear if the test fails. Let me know if you feel strongly, I can change it.
Contributor
There was a problem hiding this comment.
Sure no worries, not a strong point.
| TEST_UTIL.waitFor(100000, (Waiter.Predicate<Exception>) () -> | ||
| registry.getParsedMasterServers().size() == 2); | ||
| final Set<ServerName> newMasters2 = registry.getParsedMasterServers(); | ||
| assertEquals(2, newMasters2.size()); |
Contributor
There was a problem hiding this comment.
After stopping activeMaster, maybe add an extra check to confirm list contains one Active and one Backup?
| // RPC has failed, trigger a refresh of master end points. We can have some spurious | ||
| // refreshes, but that is okay since the RPC is not expensive and not in a hot path. | ||
| synchronized (refreshMasters) { | ||
| refreshMasters.notify(); |
Contributor
There was a problem hiding this comment.
For any generic RPC failure, we want to expedite populating masters with another RPC call.
Let's say there are some sequence of events:
- getClusterId() RPC call failed
- master refresher thread was in
waitingstate, so we notify it and it will trigger getMasters() call - the call fails again and we
notifyrefreshMasters but no one is waiting on it, notify is ignored - master refresher thread again waits for 5 min before populating masters.
Do we really want step 4 to wait for 5 min (assuming no other RPC call happens and masters list is stale)? Maybe we can expedite populating masters with the help of AtomicBoolean check (and also avoid synchronized + wait calls i.e 5 min wait)?
Even if we have network issue, we don't want to delay populate masters by 5 min right?
ContributorAuthor
There was a problem hiding this comment.
Maybe we can expedite populating masters with the help of AtomicBoolean check
Not sure I follow this, mind rephrasing?
assuming no other RPC call happens and masters list is stale
If not other RPC call happens, it doesn't matter if the list is stale or not?
Even if we have network issue, we don't want to delay populate masters by 5 min right?
Not sure I follow, if we have a network issue, how can we populate?
Contributor
There was a problem hiding this comment.
Maybe we can expedite populating masters with the help of AtomicBoolean check
Not sure I follow this, mind rephrasing?
I meant to say if refresh thread misses this notify because it is already done waiting on refreshMasters, for the next loop, it should not again wait 5 min on refreshMasters and rather quickly perform RPC call to populate masters.
Even if we have network issue, we don't want to delay populate masters by 5 min right?
Not sure I follow, if we have a network issue, how can we populate?
I meant same as above that even if network issue causes notify to refresh thread when it was already past waiting state, maybe next time the thread better quickly make an RPC call rather than waiting 5 min on refreshMasters. But yes, for network issues, we will keep making RPC calls without any progress.
If not other RPC call happens, it doesn't matter if the list is stale or not?
Hmm that's true.
ContributorAuthor
There was a problem hiding this comment.
I meant to say if refresh thread misses this notify because it is already done waiting on refreshMasters, for the next loop, it should not again wait 5 min on refreshMasters and rather quickly perform RPC call to populate masters.
I don't think thats needed. If the thread has just fetched the masters (in cases where it missed the notification), it is very unlikely that something new has been added/removed. Typically this is a very rare event, probably less rare in K8s environment than DC deployments but even then I don't think things usually change for days if not weeks.
bharathv
commented
Jul 24, 2020
bharathv
left a comment
ContributorAuthor
There was a problem hiding this comment.
PR should point to splittable meta feature branch?
No, this has nothing to do with splittable meta. @saintstack just pointed out this optimization in the design doc that I thought would be generally useful to any who wants to use master registry. This was also discussed in the parent jira HBASE-18095 too.
The goal of the patch is keep the client's local master end points up-to-date without operators having to worry about refreshing the client configuration after adding/deleting new master roles. This optimization would be useful generally.
I will address the other comments once we iron out the design and everyone is okay with the approach.
| // RPC has failed, trigger a refresh of master end points. We can have some spurious | ||
| // refreshes, but that is okay since the RPC is not expensive and not in a hot path. | ||
| synchronized (refreshMasters) { | ||
| refreshMasters.notify(); |
ContributorAuthor
There was a problem hiding this comment.
Maybe we can expedite populating masters with the help of AtomicBoolean check
Not sure I follow this, mind rephrasing?
assuming no other RPC call happens and masters list is stale
If not other RPC call happens, it doesn't matter if the list is stale or not?
Even if we have network issue, we don't want to delay populate masters by 5 min right?
Not sure I follow, if we have a network issue, how can we populate?
Apache-HBase
commented
Jul 26, 2020
🎊 +1 overall
This message was automatically generated. |
virajjasani
left a comment
Contributor
There was a problem hiding this comment.
@saintstack just pointed out this optimization in the design doc that I thought would be generally useful to any who wants to use master registry.
Yes, sounds good. Thanks @bharathv .
| // RPC has failed, trigger a refresh of master end points. We can have some spurious | ||
| // refreshes, but that is okay since the RPC is not expensive and not in a hot path. | ||
| synchronized (refreshMasters) { | ||
| refreshMasters.notify(); |
Contributor
There was a problem hiding this comment.
Maybe we can expedite populating masters with the help of AtomicBoolean check
Not sure I follow this, mind rephrasing?
I meant to say if refresh thread misses this notify because it is already done waiting on refreshMasters, for the next loop, it should not again wait 5 min on refreshMasters and rather quickly perform RPC call to populate masters.
Even if we have network issue, we don't want to delay populate masters by 5 min right?
Not sure I follow, if we have a network issue, how can we populate?
I meant same as above that even if network issue causes notify to refresh thread when it was already past waiting state, maybe next time the thread better quickly make an RPC call rather than waiting 5 min on refreshMasters. But yes, for network issues, we will keep making RPC calls without any progress.
If not other RPC call happens, it doesn't matter if the list is stale or not?
Hmm that's true.
| private final RpcControllerFactory rpcControllerFactory; | ||
| private final int rpcTimeoutMs; | ||
| // For synchronizing on refreshing the master end-points | ||
| private final Object refreshMasters = new Object(); |
Contributor
There was a problem hiding this comment.
nit: static final here? Anyways, MasterRegistry is singleton right? (if not by design, but by usage)
ContributorAuthor
There was a problem hiding this comment.
No, it's loosely tied to a single connection (there are some places that create on the fly registries which is something that can be fixed).
A single application can connect to multiple clusters which means we cannot make it static.
Contributor
There was a problem hiding this comment.
Oops, yes. This better be just final.
Apache-HBase
commented
Jul 26, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 26, 2020
🎊 +1 overall
This message was automatically generated. |
bharathv
commented
Jul 28, 2020
bharathv
left a comment
ContributorAuthor
There was a problem hiding this comment.
Anyone else has any thoughts on the approach this patch takes? Implementation is pretty straight forward but happy to add more detail if needed.
| // RPC has failed, trigger a refresh of master end points. We can have some spurious | ||
| // refreshes, but that is okay since the RPC is not expensive and not in a hot path. | ||
| synchronized (refreshMasters) { | ||
| refreshMasters.notify(); |
ContributorAuthor
There was a problem hiding this comment.
I meant to say if refresh thread misses this notify because it is already done waiting on refreshMasters, for the next loop, it should not again wait 5 min on refreshMasters and rather quickly perform RPC call to populate masters.
I don't think thats needed. If the thread has just fetched the masters (in cases where it missed the notification), it is very unlikely that something new has been added/removed. Typically this is a very rare event, probably less rare in K8s environment than DC deployments but even then I don't think things usually change for days if not weeks.
ndimiduk
reviewed
Aug 3, 2020
| public void callMethod(MethodDescriptor method, RpcController controller, Message request, | ||
| Message responsePrototype, RpcCallback<Message> done) { | ||
| if (!method.getName().equals("GetClusterId")) { | ||
| // Master registry internally runs other RPCs to keep the master list up to date. This check |
Member
There was a problem hiding this comment.
Can you say more here? Why is it that the internal RPCs that keep the master list up to date are sufficient to skip a call to "GetClusterId"? Can you provide a "see also" comment that points the reader off to the counting logic, or at least the counter that this condition protects?
ContributorAuthor
There was a problem hiding this comment.
Will add more detail. That is needed because of the way the test is written. This RpcChannel implementation intercepts all the mock RPCs from unit tests and the just counts the getClusterId calls (depending on the index).. With the patch a single GetClusterID() RPC failure can trigger an extra getMasters() call and that is accounted too.
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.
| refreshMasters.wait(WAIT_TIME_OUT_MS); | ||
| } | ||
| LOG.debug("Attempting to refresh master address end points."); | ||
| Set<ServerName> newMasters = new HashSet<>(getMasters().get()); |
Member
There was a problem hiding this comment.
Where would I find metrics regarding calls to getMasters()? I suppose either client or server-side would be good.
ContributorAuthor
There was a problem hiding this comment.
I haven't thought about this. Are you talking about this RPC specifically? If so, may I ask why?
Adding it on the server makes sense to me, like we want some metrics around where most of the time is spent (grouped by RPC) but curious what purpose it serves on client.
Member
There was a problem hiding this comment.
Someone who has the HBase client embedded in their application might be interested in observing this behavior, if they notice sudden spikes in RPC traffic not directly correlated with their application's data path.
Or maybe it's enough to track these calls on the server-side. We'd see the same spikes, though with less granularity.
ContributorAuthor
There was a problem hiding this comment.
Okay. Looks like we have all the plumbing for client metrics already in place via HBASE-12911. I can add metrics for this RPC.
Member
There was a problem hiding this comment.
Either client-side or server-side is fine with me. Just having something that an operator can expose would be useful.
Apache-HBase
commented
Aug 4, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 4, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 4, 2020
💔 -1 overall
This message was automatically generated. |
Uh oh!
There was an error while loading. Please reload this page.
bharathv
commented
Aug 10, 2020
bharathv
left a comment
ContributorAuthor
There was a problem hiding this comment.
Added metrics, some more unit test coverage and refactored the code slightly.
Apache-HBase
commented
Aug 10, 2020
Apache-HBase
commented
Aug 10, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 10, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 14, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 14, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 14, 2020
💔 -1 overall
This message was automatically generated. |
bharathv
commented
Aug 17, 2020
bharathv
left a comment
ContributorAuthor
There was a problem hiding this comment.
think this should be done in this patch, as now this method will replace the old getActiveMaster method, which makes it not only be used in our internal refresh but also be used by end users, we should not let users still have the ability to harmmer zookeeper...
I have a separate patch for this, didn't want to scope creep this one. Mind if do a separate PR? I will back port them together.
Ended up implementing caching too in the same patch, for completeness sake.
Apache-HBase
commented
Aug 17, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 17, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 17, 2020
🎊 +1 overall
This message was automatically generated. |
Apache9
requested changes
Aug 20, 2020
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bharathv
commented
Aug 24, 2020
ContributorAuthor
@Apache9 / @virajjasani Any more comments or is this good to go? This has been open for a while, would like to get it in asap unless you have any comments. |
Apache-HBase
commented
Aug 24, 2020
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Aug 24, 2020
Contributor
The only concern is about the IllegalStateException. We used to throw IOException for this case. And please fix the checkstyle issues? |
Apache-HBase
commented
Aug 24, 2020
🎊 +1 overall
This message was automatically generated. |
Comment on lines
+105
to
+108
| periodicRefreshMs = 1000 * conf.getLong(PERIODIC_REFRESH_INTERVAL_SECS, | ||
| PERIODIC_REFRESH_INTERVAL_SECS_DEFAULT); | ||
| timeBetweenRefreshesMs = 1000 * conf.getLong(MIN_SECS_BETWEEN_REFRESHES, | ||
| MIN_SECS_BETWEEN_REFRESHES_DEFAULT); |
Contributor
There was a problem hiding this comment.
nit: TimeUnit.SECONDS.toMillis(conf.getLong(,))
| private static final int PERIODIC_REFRESH_INTERVAL_SECS_DEFAULT = 300; | ||
| public static final String MIN_SECS_BETWEEN_REFRESHES = | ||
| "hbase.client.master_registry.min_secs_between_refreshes"; | ||
| private static final long MIN_SECS_BETWEEN_REFRESHES_DEFAULT = 60; |
Contributor
There was a problem hiding this comment.
nit: keep this int similar to PERIODIC_REFRESH_INTERVAL_SECS_DEFAULT?
| registry.populateMasterStubs(newMasters); | ||
| LOG.debug("Finished refreshing master end points. {}", newMasters); | ||
| } catch (InterruptedException e) { | ||
| LOG.debug("Interrupted during wait, aborting refresh-masters-thread.", e); |
Contributor
There was a problem hiding this comment.
We are aborting refresh by breaking out of the loop and basically, we are done refreshing master stubs. Better to log this at ERROR?
ContributorAuthor
There was a problem hiding this comment.
That also happens during a regular pool shutdown. Added another log later in the method after loop exist for observability.
| zkw.getZNodePaths().backupMasterAddressesZNode); | ||
| } catch (KeeperException e) { | ||
| LOG.warn(zkw.prefix("Unable to list backup servers"), e); | ||
| backupMasterStrings = null; |
Contributor
There was a problem hiding this comment.
nit: return Collections.emptyList();
virajjasani
commented
Aug 24, 2020
Contributor
Keeping backupMasters volatile list up-to date in ActiveMasterManager is nice move with this PR. |
Apache-HBase
commented
Aug 24, 2020
🎊 +1 overall
This message was automatically generated. |
bharathv
commented
Aug 24, 2020
ContributorAuthor
I added a checked exception. TBH I don't fully understand the concern. Like I explained it happens only if an invariant fails, which means a code bug, so unchecked exception should be fine. Either way its not a big deal what exception we throw, so made the changed. Also, fixed the check style issues. |
Apache-HBase
commented
Aug 24, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2020
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Aug 25, 2020
This patch adds the ability to discover newly added masters dynamically on the master registry side. The trigger for the re-fetch is either periodic (5 mins) or any registry RPC failure. Master server information is cached in masters to avoid repeated ZK lookups. Updates the client side connection metrics to maintain a counter per RPC type so that clients have visibility into counts grouped by RPC method name. I didn't add the method to ZK registry interface since there is a design discussion going on in splittable meta doc. We can add it later if needed. Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
Apache-HBase
commented
Aug 25, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 25, 2020
🎊 +1 overall
This message was automatically generated. |
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.
This patch adds the ability to discover newly added masters
dynamically on the master registry side. The trigger for the
re-fetch is either 5mins or any registry RPC failure.
I didn't add the method to ZK registry interface since there
is a design discussion going on in splittable meta doc. We can
add it later if needed.