Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12158 )
Change subject: KUDU-2348: java client pick a random replica when no replica is local ...................................................................... Patch Set 6: (11 comments) http://gerrit.cloudera.org:8080/#/c/12158/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12158/6//COMMIT_MSG@7 PS6, Line 7: KUDU-2348: java client pick a random replica when no replica is local nit: Patch up the subject a little: KUDU-2348: In the Java client, pick a random replica when no replica is local http://gerrit.cloudera.org:8080/#/c/12158/6//COMMIT_MSG@9 PS6, Line 9: return returns http://gerrit.cloudera.org:8080/#/c/12158/6//COMMIT_MSG@10 PS6, Line 10: that would cause which may cause http://gerrit.cloudera.org:8080/#/c/12158/6//COMMIT_MSG@11 PS6, Line 11: tabletservers tablet servers http://gerrit.cloudera.org:8080/#/c/12158/6//COMMIT_MSG@11 PS6, Line 11: some one http://gerrit.cloudera.org:8080/#/c/12158/6//COMMIT_MSG@12 PS6, Line 12: use a more random approach to select server I think there should be more detail about what the new approach is and how it does and does not affect the current situation. The current situation is: when the client chooses a replica to scan, and there are no local replicas, it chooses whichever server ends up last in the map iteration order, every time. So the choice is determined by the set of UUIDs of tablet servers hosting replicas and implementation details: the map implementation (consider if we used a TreeMap instead of a hashmap) and possibly the history of the map instance. As the issue said, this is bad because it could be the same across many client instances talking to the cluster. The new situation is: when the client chooses a replica to scan, and there are no replicas to scan, it chooses one based on the first character of the tablet id and the map iteration order. For a fixed tablet, different clients will make the same choice of server assuming they have the same map iteration order. This is an improvement over the current situation, but there is still the possibility for clients to pile on to one server for all the scans of a particular tablet. A complete solution to the problem would remove the map iteration order as a factor and choose a server at random without regard for tablet id, so clients will scan different servers for the same tablet even if their 'tabletServers' map instances have exactly the same state. Unfortunately, that's trickier, because right now the Java client is dependent on 'getClosestServerInfo' returning the same choice for the same set of tablet servers. See 'testGetReplicaSelectedServerInfoDeterminism' in TestRemoteTablet.java. http://gerrit.cloudera.org:8080/#/c/12158/6/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java File java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java: http://gerrit.cloudera.org:8080/#/c/12158/6/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@186 PS6, Line 186: t( Missing space. http://gerrit.cloudera.org:8080/#/c/12158/6/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@187 PS6, Line 187: If none is local server If no server is local http://gerrit.cloudera.org:8080/#/c/12158/6/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@192 PS6, Line 192: if (index == randomIndex) { : result = e; : } : if (e.isLocal()) { : return e; : } For clarity, I'd prefer the ifs to be in "priority order", e.g. for (ServerInfo e : tabletServers.values()) { if (e.isLocal()) { return e; } if (index == randomIndex) { result = e; } } http://gerrit.cloudera.org:8080/#/c/12158/6/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@198 PS6, Line 198: Extra space. http://gerrit.cloudera.org:8080/#/c/12158/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java: http://gerrit.cloudera.org:8080/#/c/12158/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java@147 PS6, Line 147: // This test ensures that remains true. Redundant. Remove. -- To view, visit http://gerrit.cloudera.org:8080/12158 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d70e45d4c9532bb32223c1dddd0936b4ff8fd99 Gerrit-Change-Number: 12158 Gerrit-PatchSet: 6 Gerrit-Owner: Zhang Yifan <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Reviewer: Zhang Yifan <[email protected]> Gerrit-Comment-Date: Fri, 04 Jan 2019 18:06:05 +0000 Gerrit-HasComments: Yes
