You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/05/30 21:31:09 UTC

[kudu-CR] WIP: Integrate the request tracker with the client

Todd Lipcon has posted comments on this change.

Change subject: WIP: Integrate the request tracker with the client
......................................................................


Patch Set 7:

(2 comments)

would be nice to have some kind of test for this at the rpc layer (against a test server or somesuch)

http://gerrit.cloudera.org:8080/#/c/3080/7/src/kudu/rpc/retriable_rpc.h
File src/kudu/rpc/retriable_rpc.h:

Line 188:   Status s = request_tracker_->FirstIncomplete(&first_incomplete);
why not have FirstIncomplete just return NO_SEQ_NO instead of a status?


Line 210:   request_tracker_->RpcCompleted(sequence_number_);
in the middle of a comment?

can you ad some kind of assertion on the destructor that this has gotten called? may require an extra boolean or state enum as an instance member, but this seems like the kind of thing we could very easily accidentally leak


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I94207c294452fcbdb3a7901fdb702674d47553ee
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes