Todd Lipcon has posted comments on this change.

Change subject: KUDU-687: expose additional tablet metadata in C++ client
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4146/1/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 488:     NON_PARTICIPANT = 3,
how come this one has a value but not the others?


Line 495:   Role role() const;
what's the intended user use case for this one? maybe we can offer something a 
bit more 'semantic'? otherwise if we add other roles like 'PRE_VOTER' it'll be 
hard to do compatibly in this API


http://gerrit.cloudera.org:8080/#/c/4146/1/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

Line 243: void RemoteTablet::GetRemoteReplicas(vector<RemoteReplica>* replicas) 
const {
should clear replicas first


http://gerrit.cloudera.org:8080/#/c/4146/1/src/kudu/client/replica-internal.cc
File src/kudu/client/replica-internal.cc:

PS1, Line 39: encoding
'role type'


Line 49:     default: LOG(FATAL) << "Unexpected encoding type: " << role;
not sure this is a good idea, since if we added a new role, clients would start 
crashing


http://gerrit.cloudera.org:8080/#/c/4146/1/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:

Line 258:     // they've been moved.
you could use ElementDeleter and just call client_replicas.clear(); after 
std::moveing it (std::move guarantees that the moved-from object is in a "valid 
but undefined state", in which case clear() on it is safe


-- 
To view, visit http://gerrit.cloudera.org:8080/4146
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3cbbb1df8d3adf60425541b57e68595bbf6e92ff
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to