David Ribeiro Alves 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? Done 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 the idea was that we could keep adding request types (like the optional below). changed this to be in sequence. 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 removed this (scaffolding code) 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 accompa Done Line 1243: if (tracking_results) { > any way to extract this new bit of code to a separate function? this is way simpler now with the atomic bit. please take a new look. 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 ke removed 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 Done 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 Done Line 314: case REPLICATING: > some spurious changes here Done 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 Done PS17, Line 122: Requests originated on other replicas > not sure quite what you mean here. Do you mean operations which were starte Done PS17, Line 158: FailAndRespond() > hrm, is this FailAndRespond here actually responding to any RPCs in the cur the design changed and the docs were not updated. change this to reflect the new state of affairs. 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 Done 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? Done 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? Done Line 295: //LOG(INFO) << "Responding error to: " << context_->request_pb()->DebugString(); > remove Done Line 764: // attempted elsewhere. > this probably needs to be moved into tablet_peer this now is done on the replicate message for all txns. removed http://gerrit.cloudera.org:8080/#/c/3449/17/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: Line 123: const char* WRITE_METHOD_NAME = "Write"; > what's this used for? dont see it referenced elsewhere Done -- 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
