Alexey Serbin 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: (6 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 location string's hierarchical structure? 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? 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()? 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 whether we want results or this random choice to be random from one run to another. 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 aggregated member 'random_'; maybe use it here instead of creating a new one? 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? -- 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 <wdberke...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Tue, 08 Jan 2019 06:39:54 +0000 Gerrit-HasComments: Yes
