[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

["Adar Dembo (Code Review)" <ge...@cloudera.org>]

Adar Dembo 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 1:

(4 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.push_back(rts);
Nit: emplace_back for new code.


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 justification would be good to add to the commit message.


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 location of a tserver is no longer immutable. That's true to the extent that a location could change from empty (i.e. unassigned) to "some location", but once a location has been assigned, it may not change to some other location, right?

As written such a change is legal, but that isn't allowed by the server, right?


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 file so it's hard to review just the changes.



-- 
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: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 02 Jan 2019 18:16:05 +0000
Gerrit-HasComments: Yes