Todd Lipcon has posted comments on this change.

Change subject: Integrate the result tracker with writes
......................................................................


Patch Set 17:

(18 comments)

http://gerrit.cloudera.org:8080/#/c/3449/17/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:

Line 51: #include "kudu/rpc/rpc_header.pb.h"
unused?


http://gerrit.cloudera.org:8080/#/c/3449/17/src/kudu/consensus/consensus.proto
File src/kudu/consensus/consensus.proto:

Line 182:   optional rpc.RequestIdPB request_id = 100;
why using such a high id here? seems like it wastes a byte


http://gerrit.cloudera.org:8080/#/c/3449/17/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

Line 162:     completion_record->final_driver = true;
this is a big code smell -- this function should be read-only, not somehow have 
a side effect. If you want it to have a side effect, rename it to 
'BecomeFinalDriver' or something? 'UpgradeToFinalDriver'?

Also, the docs in the header file should reference this concept of being the 
'final driver'


http://gerrit.cloudera.org:8080/#/c/3449/17/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:

Line 226:     bool final_driver = false;
doc this

also, weird that this field has a default but the others don't


http://gerrit.cloudera.org:8080/#/c/3449/17/src/kudu/tablet/tablet_bootstrap.cc
File src/kudu/tablet/tablet_bootstrap.cc:

Line 251:                              WriteResponsePB* response);
mentioned on rev 1, but I dont like the side effect here without an 
accompanying method rename and doc update


Line 1243:   if (tracking_results) {
any way to extract this new bit of code to a separate function?


Line 1279:       LOG(FATAL) << "Couldn't change bootstrapping txn to driver 
after 10 attempts for request: "
this seems like a kind of arbitrary thing... should this be a DFATAL and keep 
looping?


http://gerrit.cloudera.org:8080/#/c/3449/17/src/kudu/tablet/tablet_peer.cc
File src/kudu/tablet/tablet_peer.cc:

Line 130:                         const scoped_refptr<ResultTracker> 
result_tracker,
reference


http://gerrit.cloudera.org:8080/#/c/3449/17/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:

Line 212:   if (state()->are_results_tracked()) {
change to if (!...) return


Line 314:     case REPLICATING:
some spurious changes here


http://gerrit.cloudera.org:8080/#/c/3449/17/src/kudu/tablet/transactions/transaction_driver.h
File src/kudu/tablet/transactions/transaction_driver.h:

PS17, Line 117: // 1 - When becoming leader, a replica has already prepared all 
the requests that it received
              : //     from the previous leader that it will try to 
replicate/commit itself.
              : /
not quite understanding this sentence


PS17, Line 122: Requests originated on other replicas 
not sure quite what you mean here. Do you mean operations which were started 
while the node is a follower? Or "originated" meaning "the first attempt"?


PS17, Line 158: FailAndRespond() 
hrm, is this FailAndRespond here actually responding to any RPCs in the current 
design? Can we encapsulate this "precedence" behavior into something like the 
'UpgradeToExclusiveDriver' call I mentioned in the other review?


http://gerrit.cloudera.org:8080/#/c/3449/17/src/kudu/tablet/transactions/write_transaction.h
File src/kudu/tablet/transactions/write_transaction.h:

PS17, Line 196: c_
typo


http://gerrit.cloudera.org:8080/#/c/3449/17/src/kudu/tserver/remote_bootstrap_session-test.cc
File src/kudu/tserver/remote_bootstrap_session-test.cc:

Line 137:                                  dummy,
can just use scoped_refptr<rpc::ResultTracker>(), no?


http://gerrit.cloudera.org:8080/#/c/3449/17/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

Line 280: const char* ALREADY_PRESENT_ERROR = "There's already an attempt at 
the same operation "
is this used?


Line 295:       //LOG(INFO) << "Responding error to: " << 
context_->request_pb()->DebugString();
remove


Line 764:   // attempted elsewhere.
this probably needs to be moved into tablet_peer


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1fa2f8db33653960f4749237b8993baba0929893
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
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