keith-turner commented on code in PR #4880:
URL: https://github.com/apache/accumulo/pull/4880#discussion_r1757704465
##########
core/src/main/java/org/apache/accumulo/core/rpc/clients/TServerClient.java:
##########
@@ -79,18 +83,30 @@ default Pair<String,C> getThriftServerConnection(Logger
LOG, ThriftClientTypes<C
final ZooCache zc = context.getZooCache();
final List<String> serverPaths = new ArrayList<>();
- zc.getChildren(tserverZooPath).forEach(tserverAddress -> {
- serverPaths.add(tserverZooPath + "/" + tserverAddress);
- });
- if (type == ThriftClientTypes.CLIENT) {
- zc.getChildren(sserverZooPath).forEach(sserverAddress -> {
- serverPaths.add(sserverZooPath + "/" + sserverAddress);
- });
+ if (type == ThriftClientTypes.CLIENT && preferredClientHost != null) {
+ // add all three paths to the set even though they may not be correct.
+ // The entire set will be checked in the code below to validate
+ // that the path is correct and the lock is held and will return the
+ // correct one.
+ serverPaths.add(tserverZooPath + "/" + preferredClientHost);
Review Comment:
Is it expected that `preferredClientHost` would include a port? Do the
paths in ZK include a port? I am not sure how the port factors into the later
code that looks something up in zookeeper.
##########
core/src/main/java/org/apache/accumulo/core/rpc/clients/TServerClient.java:
##########
@@ -54,6 +54,8 @@
public interface TServerClient<C extends TServiceClient> {
+ static final String PREFERRED_HOST =
"org.apache.accumulo.client.rpc.preferred.host";
Review Comment:
Not advocating for this, but was wondering if this could be an accumulo
client property instead of a java system property. The only advantage I can
think of for this is that it makes documenting this easier in that we auto
generate docs for the client props.
The advantage of the system prop is that it seems a bit easier to apply at
runtime.
##########
core/src/main/java/org/apache/accumulo/core/rpc/clients/TServerClient.java:
##########
@@ -113,6 +129,9 @@ default Pair<String,C> getThriftServerConnection(Logger
LOG, ThriftClientTypes<C
TTransport transport =
context.getTransportPool().getTransport(type,
tserverClientAddress, rpcTimeout, context,
preferCachedConnections);
C client = ThriftUtil.createClient(type, transport);
+ if (type == ThriftClientTypes.CLIENT && preferredClientHost !=
null) {
Review Comment:
If the preferred host does not exists, should it fall back to a random
server? If we don't want fallback could change the prop name to required host.
##########
core/src/main/java/org/apache/accumulo/core/rpc/clients/TServerClient.java:
##########
@@ -54,6 +54,8 @@
public interface TServerClient<C extends TServiceClient> {
+ static final String PREFERRED_HOST =
"org.apache.accumulo.client.rpc.preferred.host";
+
Pair<String,C> getThriftServerConnection(ClientContext context, boolean
preferCachedConnections)
Review Comment:
Could add an IT that directly calls this method a few times with and without
the preferred host prop set and see what it returns.
--
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]
