Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16992 )
Change subject: KUDU-2612: loosen restrictions for BEGIN_COMMIT ops ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/16992/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16992/1//COMMIT_MSG@17 PS1, Line 17: commit task: > nit: remove this extra? Done http://gerrit.cloudera.org:8080/#/c/16992/1/src/kudu/integration-tests/txn_participant-itest.cc File src/kudu/integration-tests/txn_participant-itest.cc: http://gerrit.cloudera.org:8080/#/c/16992/1/src/kudu/integration-tests/txn_participant-itest.cc@568 PS1, Line 568: TabletServerErrorPB::Code code = TabletServerErrorPB::UNKNOWN_ERROR; : ASSERT_OK(ParticipateInTransactionCheckResp( : admin_proxy.get(), tablet_id, kTxnId, ParticipantOpPB::BEGIN_COMMIT, : &code, &begin_commit_ts)); : ASSERT_EQ(TabletServerErrorPB::UNKNOWN_ERROR, code); : ASSERT_NE(Timestamp::kInvalidTimestamp, begin_commit_ts); > What if BEGIN_COMMIT is called twice prior calling FINALIZE_COMMIT? Will i Right, this is tested at L645. http://gerrit.cloudera.org:8080/#/c/16992/1/src/kudu/tablet/txn_participant.h File src/kudu/tablet/txn_participant.h: http://gerrit.cloudera.org:8080/#/c/16992/1/src/kudu/tablet/txn_participant.h@135 PS1, Line 135: ValidateBeginCommit > Each criterion on every possible state here is wrapped into PREDICT_FALSE. Yeah it's intentional; the Status::OK case is the common case. http://gerrit.cloudera.org:8080/#/c/16992/1/src/kudu/tablet/txn_participant.h@145 PS1, Line 145: meta_state != kCommitted && meta_state != kCommitInProgress > nit: does it make sense to wrap this into PREDICT_FALSE() ? Done http://gerrit.cloudera.org:8080/#/c/16992/1/src/kudu/tablet/txn_participant.h@151 PS1, Line 151: already_applied_timestamp = timestamp; > nit: could you add a comment explaining what is the possible scenarios to g Done http://gerrit.cloudera.org:8080/#/c/16992/1/src/kudu/tablet/txn_participant.h@156 PS1, Line 156: already_applied_timestamp > nit: does it make sense to check for the validity of the timestamp here? Done -- To view, visit http://gerrit.cloudera.org:8080/16992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifa4c5314190c84648c1b1edea7aab776b4882f97 Gerrit-Change-Number: 16992 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 30 Jan 2021 07:50:07 +0000 Gerrit-HasComments: Yes
