Alexey Serbin 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? 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 it return the same timestamp? I guess it's supposed it should. If so indeed, does it make sense to add a sub-scenario for that? 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. Is it intended? Not sure I see how it can make sense. 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() ? 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 get up to this point? 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? -- 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: Fri, 29 Jan 2021 05:42:10 +0000 Gerrit-HasComments: Yes
