virajjasani commented on a change in pull request #2130:
URL: https://github.com/apache/hbase/pull/2130#discussion_r460552343
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MasterRegistry.java
##########
@@ -170,6 +214,11 @@ public static String getMasterAddr(Configuration conf)
throws UnknownHostExcepti
callable.call(controller, stub, resp -> {
if (controller.failed()) {
future.completeExceptionally(controller.getFailed());
+ // 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();
Review 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.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MasterRegistry.java
##########
@@ -89,11 +97,17 @@
private final int hedgedReadFanOut;
// Configured list of masters to probe the meta information from.
- private final ImmutableMap<ServerName, ClientMetaService.Interface>
masterAddr2Stub;
+ private volatile ImmutableMap<ServerName, ClientMetaService.Interface>
masterAddr2Stub;
// RPC client used to talk to the masters.
private final RpcClient rpcClient;
private final RpcControllerFactory rpcControllerFactory;
+ private final int rpcTimeoutMs;
+ // For synchronizing on refreshing the master end-points
+ private final Object refreshMasters = new Object();
Review comment:
nit: `static final` here? Anyways, `MasterRegistry` is singleton right?
(if not by design, but by usage)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
