Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16879 )
Change subject: KUDU-2612: add RPC to send participant ops ...................................................................... Patch Set 4: (8 comments) Overall looks good to me, just few nits and a couple of question for the newly added tests scenarios. http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/integration-tests/txn_participant-itest.cc File src/kudu/integration-tests/txn_participant-itest.cc: http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/integration-tests/txn_participant-itest.cc@782 PS4, Line 782: MonoDelta::FromSeconds(10) nit for here and other test scenarios added: make this a scenario's scope-wide constant? http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/integration-tests/txn_participant-itest.cc@815 PS4, Line 815: BEGIN_COMMIT Does it make sense to add a sub-scenario to show what happens when calling ABORT at this stage? What's the expected status? http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/integration-tests/txn_participant-itest.cc@896 PS4, Line 896: Restart the down servers and check that we get to the right state. What would happen if the leader replica that keeps the information about the transaction in run-time is paused, the rest two replicas are restarted and one of the restarted replicas becomes a leader, and the txn_client somehow retries and calls ParticipateInTransaction() with the same txn_id but with the new leader? What happens what the former leader is resumed/unfrozen? Will the state of the former leader replica will be consistent, so that on next re-election it would think of the new transaction state as of kOpen, not kInitializing? http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/integration-tests/txn_participant-itest.cc@1215 PS4, Line 1215: const nit: might it be constexpr ? http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/integration-tests/txn_participant-itest.cc@1221 PS4, Line 1221: ParticipantOpPB::BEGIN_TXN, : ParticipantOpPB::BEGIN_COMMIT, : ParticipantOpPB::ABORT_TXN }) nit: does it make to separate this into a member field like kCommitSequence? http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/transactions/participant_rpc.h File src/kudu/transactions/participant_rpc.h: http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/transactions/participant_rpc.h@56 PS4, Line 56: // NOTE: if 'op_result' is non-null, Update the comment? http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/transactions/participant_rpc.cc File src/kudu/transactions/participant_rpc.cc: http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/transactions/participant_rpc.cc@76 PS4, Line 76: ParticipantRpc* rpc = new ParticipantRpc(std::move(ctx), : std::move(server_picker), : deadline, : std::move(user_cb), : begin_commit_timestamp); : return rpc; nit: why not just return new ParticipantRpc() ... ? http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/transactions/txn_system_client.cc File src/kudu/transactions/txn_system_client.cc: http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/transactions/txn_system_client.cc@321 PS4, Line 321: ctx_raw Since we have switched to C++17 from C++11, maybe it's time to start using move-capture here for 'ctx' as well (I guess it should have been available in C++14 already)? -- To view, visit http://gerrit.cloudera.org:8080/16879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb9ba09104761772f9aaffe582776ad34d8dbf57 Gerrit-Change-Number: 16879 Gerrit-PatchSet: 4 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-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 18 Dec 2020 06:44:12 +0000 Gerrit-HasComments: Yes
