Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12138 )
Change subject: Support location awareness in READ_CLOSEST for the C++ client ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@406 PS7, Line 406: random > nit: does it make sense to add a TODO for the future to make use of the loc Done http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@408 PS7, Line 408: vector<RemoteTabletServer*> local; : vector<RemoteTabletServer*> same_location; > nit: call reserve(filtered.size()) for local and same_location? Done http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@414 PS7, Line 414: const string replica_location = rts->location(); > nit: maybe, gate computing/calling this based on !client_location.empty()? Done http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@422 PS7, Line 422: rand > Do we init random generator in the client library? I guess my question is After looking at this with Alexey, we think we don't seed rand(), but we should be. He said he would look at it as a separate changelist. http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc File src/kudu/integration-tests/location_assignment-itest.cc: http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc@79 PS7, Line 79: ThreadSafeRandom rng(SeedRandom()); > nit: it seems TabletServerIntegrationTestBase has ThreadSafeRandom as aggre Done http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc@82 PS7, Line 82: auto location = Substitute("/L$0", i); > Now unused. Done http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc@125 PS7, Line 125: // When running on Linux, the client and tablet servers each have their own : // IP in the local address space, so no tablet server will be considered : // local to the client. If there is a tablet server in the same location as : // the client, it will be the only tablet server scanned. Otherwise, some : // random tablet server will be scanned. > nit: move this under the #if defined block below? Done -- To view, visit http://gerrit.cloudera.org:8080/12138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713 Gerrit-Change-Number: 12138 Gerrit-PatchSet: 7 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 08 Jan 2019 18:58:03 +0000 Gerrit-HasComments: Yes
