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 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client-internal.cc@412 PS1, Line 412: local.emplace_back(rts); > Nit: emplace_back for new code. Done http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client.h@472 PS1, Line 472: ///< Local replicas are considered the closest, : ///< followed by replicas in the same location as the : ///< client, followed by all other replicas. If there are : ///< multiple closest replicas, one is chosen randomly. > Would you consider this to be a breaking change? The answer and justificati Done http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/meta_cache.h File src/kudu/client/meta_cache.h: http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/meta_cache.h@109 PS1, Line 109: // Return a copy of this tablet server's location, as assigned by the master. > The changes here (removal of constness, returning a copy) suggest that the No, the location can change from the point of view of the client. This might happen, for example, if the cluster configuration is changed during the lifetime of a long-lived client instance. Once the server assigns a location to a tablet server, the location will not change unless the tablet server re-registers, which AFAIK happens only when the tablet server restarts, or if the master restarts and the tablet server needs to register with the new master process. The location could also change if the leader master changes and the location mapping command gave different results on the new leader master than on the old one for the tserver, but this is really a configuration problem. http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/integration-tests/location_assignment-itest.cc File src/kudu/integration-tests/location_assignment-itest.cc: PS1: > Could you rename this in a separate commit? It's showing up as a brand new Done http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/integration-tests/location_assignment-itest.cc@51 PS1, Line 51: > warning: using decl 'KuduClientBuilder' is unused [misc-unused-using-decls] 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: 2 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 04 Jan 2019 19:48:34 +0000 Gerrit-HasComments: Yes
