bharathv commented on a change in pull request #3566:
URL: https://github.com/apache/hbase/pull/3566#discussion_r687025151
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
##########
@@ -308,18 +310,35 @@
*/
private static final long
DEFAULT_REGION_SERVER_RPC_MINIMUM_SCAN_TIME_LIMIT_DELTA = 10;
- /*
+ /**
* Whether to reject rows with size > threshold defined by
* {@link RSRpcServices#BATCH_ROWS_THRESHOLD_NAME}
*/
private static final String REJECT_BATCH_ROWS_OVER_THRESHOLD =
"hbase.rpc.rows.size.threshold.reject";
- /*
+ /**
* Default value of config {@link
RSRpcServices#REJECT_BATCH_ROWS_OVER_THRESHOLD}
*/
private static final boolean DEFAULT_REJECT_BATCH_ROWS_OVER_THRESHOLD =
false;
+ /**
+ * Determine the bootstrap nodes we want to return to the client connection
registry.
+ * <ul>
+ * <li>{@link #MASTER}: return masters as bootstrap nodes.</li>
Review comment:
> but if users do want to use masters
I don't see how it breaks current semantics of MasterRegistry if they
continue using it. If some users are currently using Master Registry, the code
flow and request path still remains the same. Following is the code in
MasterRegistry (based on your last commit)
```
@Override
protected Set<ServerName> getBootstrapNodes(Configuration conf) throws
IOException {
return parseMasterAddrs(conf);
}
@Override
protected CompletableFuture<Set<ServerName>> fetchEndpoints() {
return getMasters();
}
```
So the flow is, initial bootrap list = whatever user configured and using
that bootstrap list we call fetchEndPoints() which return a bunch fo master
stubs (_only masters_, nothing else). Now MasterRpcServers implements the
RegistryProtos.ClientMetaService (via RsRpcServices) so we have all the methods
available and the request path is the same?
With this current change, (my understanding) RpcConnectionRegistry behavior
actually changes, there we have
```
private CompletableFuture<Set<ServerName>> getBootstrapNodes() {
return this
.<GetBootstrapNodesResponse> call(
(c, s, d) -> s.getBootstrapNodes(c,
GetBootstrapNodesRequest.getDefaultInstance(), d),
r -> r.getServerNameCount() != 0, "getBootstrapNodes()")
.thenApply(RpcConnectionRegistry::transformServerNames);
}
@Override
protected CompletableFuture<Set<ServerName>> fetchEndpoints() {
return getBootstrapNodes();
}
```
The above code can potentially return MASTERS if it is configured on the
server side. Here we are _not_ breaking anything because
`RpcConnectionRegistry` is a new addition and the semantics are to use
regionservers. As I mentioned above, MasterRegistry still uses getMasters(), so
it never sees regionservers. I don't see how anything is broken.
If someone wants to use masters, they can just use MasterRegistry until it
is deprecated in 4.0.0.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
