Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14981 )
Change subject: [java] retry to connect to the cluster when specifing a superset of masters ...................................................................... Patch Set 1: (3 comments) Could you please make the equivalent change to the C++ client too? http://gerrit.cloudera.org:8080/#/c/14981/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14981/1//COMMIT_MSG@9 PS1, Line 9: When the user specifies masters in the connection string, which contains all the : existing masters, but couldn't find a leader master, the client should retry : to connect to the cluster instead of returning with NonRecoverableException. : This will be useful when we need to add or remove masters. : I'm not really seeing how this patch achieves this. It loosens the coupling between "number of masters the client think there are" and "number of masters there actually are" such that if there are _more_ masters in reality and we weren't able to find the leader master, the client will retry the lookup instead of failing. But, why would a retry in this case yield a different answer? Suppose there are three masters in reality but the client is configured with just one of them. If ConnectToCluster can't find the leader master (i.e. the master known to the client is a follower), won't a retry fail to find the leader master too? What's the point of retrying in this case? I can see the argument for the opposite approach (i.e. retry if the client thinks there are more masters than actually exist) and that could help when removing masters. But as written, that's not what this patch is doing. http://gerrit.cloudera.org:8080/#/c/14981/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java: http://gerrit.cloudera.org:8080/#/c/14981/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java@283 PS1, Line 283: public void testConnectMoreMasters() throws Exception { I think you can include this case into the existing testAggregateResponses test rather than defining a brand new case. http://gerrit.cloudera.org:8080/#/c/14981/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java@298 PS1, Line 298: return ConnectToMasterResponsePB.newBuilder() This (or makeCTMR, or both) need better names to differentiate between their behaviors. Or makeCTMR should be refactored to accommodate this use case. -- To view, visit http://gerrit.cloudera.org:8080/14981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I66033c3ff6d217ce6b8286c94a7333b90cd26d19 Gerrit-Change-Number: 14981 Gerrit-PatchSet: 1 Gerrit-Owner: Yifan Zhang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 07 Jan 2020 22:33:20 +0000 Gerrit-HasComments: Yes
