Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16879 )
Change subject: KUDU-2612: add RPC to send participant ops ...................................................................... Patch Set 5: Verified+1 (8 comments) The test failures seem unrelated and are likely due to the recent internal failures to populate the flaky test dashboard. 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: RT_OK(txn_client->Particip > nit for here and other test scenarios added: make this a scenario's scope-w Done http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/integration-tests/txn_participant-itest.cc@815 PS4, Line 815: > Does it make sense to add a sub-scenario to show what happens when calling Done http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/integration-tests/txn_participant-itest.cc@896 PS4, Line 896: o* leader_replica = replicas[kLeaderIdx]; > What would happen if the leader replica that keeps the information about th When the old leader rejoins the quorum, it will abort ops from previous terms, and the abort path for participant ops includes a call to ClearIfInitFailed, which would remove the Txn rather than leaving it kInitializing. http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/integration-tests/txn_participant-itest.cc@1215 PS4, Line 1215: > nit: might it be constexpr ? Done http://gerrit.cloudera.org:8080/#/c/16879/4/src/kudu/integration-tests/txn_participant-itest.cc@1221 PS4, Line 1221: plica> r; : ASSERT_TRUE(cluster_->mini_tablet_server(i)->server()->tablet_manager()->LookupTablet( : tablet_id, &r)); > nit: does it make to separate this into a member field like kCommitSequence Done 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 'begin_commit_timestamp' > Update the comment? Done 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: return new ParticipantRpc(std::move(ctx), : std::move(server_picker), : deadline, : std::move(user_cb), : begin_commit_timestamp); : } > nit: why not just Done 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: ectly. > Since we have switched to C++17 from C++11, maybe it's time to start using Alas, it seems like there still won't be a great way to pass a unique_ptr into a lambda: https://taylorconor.com/blog/noncopyable-lambdas/ I tried some stuff with lambdas, but haven't hammered on it enough to get to the bottom of how to do this. I'll leave a TODO and this link as a comment. -- 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: 5 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: Mon, 21 Dec 2020 01:17:05 +0000 Gerrit-HasComments: Yes
