[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. KUDU-2612 tablet servers automatically register txn participants With this patch, tablet servers automatically register tablets as transaction participants and issue appropriate BEGIN_TXN operations upon receiving write operations targeting tablet replicas which they host. Internally, the newly introduced logic is mostly embedded into the TxnOpDispatcher class. Every TxnOpDispatcher object is allowed to accumulate up to a certain number of pending write requests in its queue while awaiting for the completion of the preliminary tasks mentioned above. Once the TxnOpDispatcher's queue is at capacity, a tablet server starts responding with the ErrorStatusPB::ERROR_SERVER_TOO_BUSY error code to incoming write requests in the context of the corresponding transaction, and Kudu clients automatically retry requests, as expected. The capacity of the TxnOpDispatcher's queue is controlled by a newly introduced --tablet_max_pending_txn_write_ops flag. By default, the flag is set to 2. Buffering a few write requests should help to avoid needless retries in case if a client packs many operations into one write request: that's exactly the behavior for Kudu client sessions using the AUTO_FLUSH_BACKGROUND mode. If such buffering isn't desired for some reason, set --tablet_max_pending_txn_write_ops=0: in that case a client will retry the very first operation sent to a tablet server in the context of a transaction until the tablet server completes the preliminary tasks mentioned above. The flag has runtime semantics, so no restart of a tablet server is required upon modification of the flag's setting. Each tablet replica maintains a txn_id --> TxnOpDispatcher map, with an entry's lifecycle as below: * An entry is added upon receiving write request in the context of a multi-row transaction. * An entry is removed upon applying either ParticipantOpPB::ABORT_TXN or ParticipantOpPB::FINALIZE_COMMIT operation. * If a write request is received after transaction has been committed or aborted, the entry is automatically removed once receiving corresponding error response from any of the following components: ** from TxnStatusManager in at attempts to register a participant in the context of committed/aborted transaction ** from the replica itself in an attempt to add ParticipantOpPB::BEGIN_COMMIT operation In other words, the system automatically gets rid of no-longer-needed TxnOpDispatcher entries. This patch also contains several test scenarios to cover the newly introduced functionality. I also updated other related tests to remove the artificial registration of corresponding transaction participants, so those tests now rely on the newly introduced functionality. Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Reviewed-on: http://gerrit.cloudera.org:8080/17037 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M src/kudu/client/batcher.cc M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/integration-tests/txn_commit-itest.cc A src/kudu/integration-tests/txn_write_ops-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 12 files changed, 2,318 insertions(+), 127 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 14 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 13: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 13 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 02 Mar 2021 23:06:34 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 13: (1 comment) http://gerrit.cloudera.org:8080/#/c/17037/13/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/13/src/kudu/tablet/tablet_replica.cc@1216 PS13, Line 1216: CHECK_OK(self->replica_->UnregisterTxnOpDispatcher( : txn_id, false/*abort_pending_ops*/)); > Do we need to call Cancel() as well? What happens to any ops in the queue? Submit() takes care of responding for those queued ops, see line 1282 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 13 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 02 Mar 2021 23:00:32 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 13: (2 comments) http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc File src/kudu/integration-tests/txn_write_ops-itest.cc: http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc@1558 PS12, Line 1558: rs between every row > One small funny nit is that this scenario is a bit different :) Ack http://gerrit.cloudera.org:8080/#/c/17037/13/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/13/src/kudu/tablet/tablet_replica.cc@1216 PS13, Line 1216: CHECK_OK(self->replica_->UnregisterTxnOpDispatcher( : txn_id, false/*abort_pending_ops*/)); Do we need to call Cancel() as well? What happens to any ops in the queue? -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 13 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 02 Mar 2021 21:52:11 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 12: (9 comments) http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc File src/kudu/integration-tests/txn_write_ops-itest.cc: http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc@227 PS12, Line 227: quals > nit: incomplete? Done http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc@648 PS12, Line 648: CHECK(d); : return d; > nit: could combine as return DCHECK_NOTNULL(d) Done http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc@1558 PS12, Line 1558: DISABLED_NonTxnWrites > Seems this was merged as https://gerrit.cloudera.org/c/17120/? One small funny nit is that this scenario is a bit different :) The essence is at line 1595: ASSERT_OK(CountRows(table_.get(), _count)); Here, some other table handle is used to read the data: 'table_', but the data was written using 'table' handle. This scenario still fails time to time, and if running in READ_YOUR_WRITES mode it fails more often (like 4 out of 256 vs 70 out of 256): 1596: Failure Expected equality of these values: 8 row_count Which is: 7 I was thinking to keep this around to clarify on this later, but I agree it's not the best place to keep it anyways, because it's not related to transactional writes or writes which are somehow affected by the newly introduced functionality. Probably, I'll need to open a separate review item for that, and corresponding upstream JIRA for that. Removed. http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.h@416 PS12, Line 416: write specified > nit: specified write operation? Done http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.h@450 PS12, Line 450: respond > nit: drop Done http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.h@460 PS12, Line 460: are > nit: dropped off? Done http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.cc@1147 PS12, Line 1147: TxnBegin > nit: BeginTxn here too? Done http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.cc@1209 PS12, Line 1209: WARN_NOT_OK(self->Submit(), "fail to submit pending txn write requests"); > What if this fails with a transient error? Seems like we may fail to submit Good catch! Indeed, it's necessary to call UnregisterTxnDispatcher in that case. Done. http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.cc@1226 PS12, Line 1226: DCHECK(!preliminary_tasks_completed_); > Should be done under lock? Done. It will be cleaner this way, right. But it's not strictly necessary since this is the only method changing the 'preliminary_tasks_completed_' field and it's to be called only once. -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 12 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 02 Mar 2021 21:24:55 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17037 to look at the new patch set (#13). Change subject: KUDU-2612 tablet servers automatically register txn participants .. KUDU-2612 tablet servers automatically register txn participants With this patch, tablet servers automatically register tablets as transaction participants and issue appropriate BEGIN_TXN operations upon receiving write operations targeting tablet replicas which they host. Internally, the newly introduced logic is mostly embedded into the TxnOpDispatcher class. Every TxnOpDispatcher object is allowed to accumulate up to a certain number of pending write requests in its queue while awaiting for the completion of the preliminary tasks mentioned above. Once the TxnOpDispatcher's queue is at capacity, a tablet server starts responding with the ErrorStatusPB::ERROR_SERVER_TOO_BUSY error code to incoming write requests in the context of the corresponding transaction, and Kudu clients automatically retry requests, as expected. The capacity of the TxnOpDispatcher's queue is controlled by a newly introduced --tablet_max_pending_txn_write_ops flag. By default, the flag is set to 2. Buffering a few write requests should help to avoid needless retries in case if a client packs many operations into one write request: that's exactly the behavior for Kudu client sessions using the AUTO_FLUSH_BACKGROUND mode. If such buffering isn't desired for some reason, set --tablet_max_pending_txn_write_ops=0: in that case a client will retry the very first operation sent to a tablet server in the context of a transaction until the tablet server completes the preliminary tasks mentioned above. The flag has runtime semantics, so no restart of a tablet server is required upon modification of the flag's setting. Each tablet replica maintains a txn_id --> TxnOpDispatcher map, with an entry's lifecycle as below: * An entry is added upon receiving write request in the context of a multi-row transaction. * An entry is removed upon applying either ParticipantOpPB::ABORT_TXN or ParticipantOpPB::FINALIZE_COMMIT operation. * If a write request is received after transaction has been committed or aborted, the entry is automatically removed once receiving corresponding error response from any of the following components: ** from TxnStatusManager in at attempts to register a participant in the context of committed/aborted transaction ** from the replica itself in an attempt to add ParticipantOpPB::BEGIN_COMMIT operation In other words, the system automatically gets rid of no-longer-needed TxnOpDispatcher entries. This patch also contains several test scenarios to cover the newly introduced functionality. I also updated other related tests to remove the artificial registration of corresponding transaction participants, so those tests now rely on the newly introduced functionality. Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 --- M src/kudu/client/batcher.cc M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/integration-tests/txn_commit-itest.cc A src/kudu/integration-tests/txn_write_ops-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 12 files changed, 2,318 insertions(+), 127 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/17037/13 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 13 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 12: (10 comments) http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc File src/kudu/integration-tests/txn_write_ops-itest.cc: http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc@227 PS12, Line 227: quals nit: incomplete? http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc@648 PS12, Line 648: CHECK(d); : return d; nit: could combine as return DCHECK_NOTNULL(d) http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/integration-tests/txn_write_ops-itest.cc@1558 PS12, Line 1558: DISABLED_NonTxnWrites Seems this was merged as https://gerrit.cloudera.org/c/17120/? http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.h@416 PS12, Line 416: write specified nit: specified write operation? http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.h@450 PS12, Line 450: respond nit: drop http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.h@460 PS12, Line 460: are nit: dropped off? http://gerrit.cloudera.org:8080/#/c/17037/10/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/10/src/kudu/tablet/tablet_replica.cc@633 PS10, Line 633: status_pb_out->set_table_ Seems like this is never anything other than OK? http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.cc@1147 PS12, Line 1147: TxnBegin nit: BeginTxn here too? http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.cc@1209 PS12, Line 1209: WARN_NOT_OK(self->Submit(), "fail to submit pending txn write requests"); What if this fails with a transient error? Seems like we may fail to submit a write because of memory pressure. If that happens, could we be left stuck with a non-OK inflight_status_, since we don't call Cancel() and UnregisterTxnOpDispatcher()? http://gerrit.cloudera.org:8080/#/c/17037/12/src/kudu/tablet/tablet_replica.cc@1226 PS12, Line 1226: DCHECK(!preliminary_tasks_completed_); Should be done under lock? -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 12 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 02 Mar 2021 08:38:41 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17037 to look at the new patch set (#12). Change subject: KUDU-2612 tablet servers automatically register txn participants .. KUDU-2612 tablet servers automatically register txn participants With this patch, tablet servers automatically register tablets as transaction participants and issue appropriate BEGIN_TXN operations upon receiving write operations targeting tablet replicas which they host. Internally, the newly introduced logic is mostly embedded into the TxnOpDispatcher class. Every TxnOpDispatcher object is allowed to accumulate up to a certain number of pending write requests in its queue while awaiting for the completion of the preliminary tasks mentioned above. Once the TxnOpDispatcher's queue is at capacity, a tablet server starts responding with the ErrorStatusPB::ERROR_SERVER_TOO_BUSY error code to incoming write requests in the context of the corresponding transaction, and Kudu clients automatically retry requests, as expected. The capacity of the TxnOpDispatcher's queue is controlled by a newly introduced --tablet_max_pending_txn_write_ops flag. By default, the flag is set to 2. Buffering a few write requests should help to avoid needless retries in case if a client packs many operations into one write request: that's exactly the behavior for Kudu client sessions using the AUTO_FLUSH_BACKGROUND mode. If such buffering isn't desired for some reason, set --tablet_max_pending_txn_write_ops=0: in that case a client will retry the very first operation sent to a tablet server in the context of a transaction until the tablet server completes the preliminary tasks mentioned above. The flag has runtime semantics, so no restart of a tablet server is required upon modification of the flag's setting. Each tablet replica maintains a txn_id --> TxnOpDispatcher map, with an entry's lifecycle as below: * An entry is added upon receiving write request in the context of a multi-row transaction. * An entry is removed upon applying either ParticipantOpPB::ABORT_TXN or ParticipantOpPB::FINALIZE_COMMIT operation. * If a write request is received after transaction has been committed or aborted, the entry is automatically removed once receiving corresponding error response from any of the following components: ** from TxnStatusManager in at attempts to register a participant in the context of committed/aborted transaction ** from the replica itself in an attempt to add ParticipantOpPB::BEGIN_COMMIT operation In other words, the system automatically gets rid of no-longer-needed TxnOpDispatcher entries. This patch also contains several test scenarios to cover the newly introduced functionality. I also updated other related tests to remove the artificial registration of corresponding transaction participants, so those tests now rely on the newly introduced functionality. Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 --- M src/kudu/client/batcher.cc M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/integration-tests/txn_commit-itest.cc A src/kudu/integration-tests/txn_write_ops-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 12 files changed, 2,285 insertions(+), 127 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/17037/12 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 12 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/17037 )
Change subject: KUDU-2612 tablet servers automatically register txn participants
..
Patch Set 9:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS9:
> I think it's better to move these new test scenarios into txn_write_ops-ite
Done
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/integration-tests/txn_write_ops-itest.cc
File src/kudu/integration-tests/txn_write_ops-itest.cc:
PS9:
> Could you also add a test in which we delete a replica that has pending ops
Done
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/integration-tests/txn_write_ops-itest.cc@446
PS9, Line 446: GetSingleTxnOpDispatcher() {
> nit: indent by 4 spaces
Done
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/ops/participant_op.cc
File src/kudu/tablet/ops/participant_op.cc:
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/ops/participant_op.cc@197
PS9, Line 197:
RETURN_NOT_OK(tablet_replica_->UnregisterTxnOpDispatcher(txn_id()));
> Apply() cannot fail, see L234 for the CHECK_OK(). I suspect we should be ab
Exactly -- as we discussed offline, the proper place for this check is
ParticipantOp::Prepare()
Done
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.h
File src/kudu/tablet/tablet_replica.h:
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.h@443
PS9, Line 443: let
> nit: give
Done
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@602
PS9, Line 602: this->RegisteredParticipantCb(txn_id, s);
> Answering my own question, no I don't think it does. That said, we could pr
Ack.
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@602
PS9, Line 602: this->RegisteredParticipantCb(txn_id, s);
> This gets set here and in the ParticipantTxnBeginCallback. Does that mean i
It's being called once, as far as I can see.
Anyway, it was messy due to iterative approach (I tried several options); I
cleaned this up a bit. Should be more straightforward now.
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@607
PS9, Line 607: SetRegistrationScheduled
> SetPreliminaryTasksScheduled()?
Done
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@1179
PS9, Line 1179: TxnParticipantBegi
> nit: BeginTxnParticipantOp? Same in the callback?
Done
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@1208
PS9, Line 1208: CancelPendingTxnOps
> Is it possible for CancelPendingTxnOps to be called twice if BEGIN_TXN op f
Nope, it's called only once as I can see. Even if it was called twice, the
second time would be a no-op because the queue of pending ops was empty.
Anyways, I cleaned it up bit -- it should be more straightforward now. I agree
it was a bit messy in this revision.
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@1260
PS9, Line 1260: preliminary_tasks_scheduled_
> If the main purpose of this is to ensure we don't have multiple registratio
It would if it were guaranteed that the caller would
(1) always try to schedule those tasks
(2) always succeed in registering tasks if it tries to do so
(1) isn't guaranteed if looking at that from API perspective. (2) isn't
guaranteed because TabletReplica::SubmitTxnWrite() can fail with non-OK status.
I agree that the current approach doesn't look particularly good but telying
on the logic of existing call sites would make even worse.
But I might be missing the essence of your question. Probably you meant just to
call the scheduler functor here and remove that call-back from the upper level
to set 'preliminary_tasks_scheduled_'? I indeed removed the extra level of
indirection, and now the scheduler function is called right from here.
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@1362
PS9, Line 1362: Cleanup
> nit: maybe call this RespondFailures() or something to make it more obvious
As for the holding the lock, I don't think it's needed here because this method
isn't supposed to be called while inflight_status_ is to change concurrently.
But it's a good call: it looks suspicious. To make it cleaner, I made this
method static.
Renamed into RespondWithStatus(), added DCHECK() for the status parameter at
the call sites.
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tserver/ts_tablet_manager.cc@1166
PS9, Line 1166:
: if
(PREDICT_FALSE(FLAGS_txn_participant_begin_op_inject_latency_ms > 0)) {
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17037 to look at the new patch set (#11). Change subject: KUDU-2612 tablet servers automatically register txn participants .. KUDU-2612 tablet servers automatically register txn participants With this patch, tablet servers automatically register tablets as transaction participants and issue appropriate BEGIN_TXN operations upon receiving write operations targeting tablet replicas which they host. Internally, the newly introduced logic is mostly embedded into the TxnOpDispatcher class. Every TxnOpDispatcher object is allowed to accumulate up to a certain number of pending write requests in its queue while awaiting for the completion of the preliminary tasks mentioned above. Once the TxnOpDispatcher's queue is at capacity, a tablet server starts responding with the ErrorStatusPB::ERROR_SERVER_TOO_BUSY error code to incoming write requests in the context of the corresponding transaction, and Kudu clients automatically retry requests, as expected. The capacity of the TxnOpDispatcher's queue is controlled by a newly introduced --tablet_max_pending_txn_write_ops flag. By default, the flag is set to 2. Buffering a few write requests should help to avoid needless retries in case if a client packs many operations into one write request: that's exactly the behavior for Kudu client sessions using the AUTO_FLUSH_BACKGROUND mode. If such buffering isn't desired for some reason, set --tablet_max_pending_txn_write_ops=0: in that case a client will retry the very first operation sent to a tablet server in the context of a transaction until the tablet server completes the preliminary tasks mentioned above. The flag has runtime semantics, so no restart of a tablet server is required upon modification of the flag's setting. Each tablet replica maintains a txn_id --> TxnOpDispatcher map, with an entry's lifecycle as below: * An entry is added upon receiving write request in the context of a multi-row transaction. * An entry is removed upon applying either ParticipantOpPB::ABORT_TXN or ParticipantOpPB::FINALIZE_COMMIT operation. * If a write request is received after transaction has been committed or aborted, the entry is automatically removed once receiving corresponding error response from any of the following components: ** from TxnStatusManager in at attempts to register a participant in the context of committed/aborted transaction ** from the replica itself in an attempt to add ParticipantOpPB::BEGIN_COMMIT operation In other words, the system automatically gets rid of no-longer-needed TxnOpDispatcher entries. This patch also contains several test scenarios to cover the newly introduced functionality. I also updated other related tests to remove the artificial registration of corresponding transaction participants, so those tests now rely on the newly introduced functionality. Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 --- M src/kudu/client/batcher.cc M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/integration-tests/txn_commit-itest.cc A src/kudu/integration-tests/txn_write_ops-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 12 files changed, 2,283 insertions(+), 127 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/17037/11 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 11 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17037 to look at the new patch set (#10). Change subject: KUDU-2612 tablet servers automatically register txn participants .. KUDU-2612 tablet servers automatically register txn participants With this patch, tablet servers automatically register tablets as transaction participants and issue appropriate BEGIN_TXN operations upon receiving write operations targeting tablet replicas which they host. Internally, the newly introduced logic is mostly embedded into the TxnOpDispatcher class. Every TxnOpDispatcher object is allowed to accumulate up to a certain number of pending write requests in its queue while awaiting for the completion of the preliminary tasks mentioned above. Once the TxnOpDispatcher's queue is at capacity, a tablet server starts responding with the ErrorStatusPB::ERROR_SERVER_TOO_BUSY error code to incoming write requests in the context of the corresponding transaction, and Kudu clients automatically retry requests, as expected. The capacity of the TxnOpDispatcher's queue is controlled by a newly introduced --tablet_max_pending_txn_write_ops flag. By default, the flag is set to 2. Buffering a few write requests should help to avoid needless retries in case if a client packs many operations into one write request: that's exactly the behavior for Kudu client sessions using the AUTO_FLUSH_BACKGROUND mode. If such buffering isn't desired for some reason, set --tablet_max_pending_txn_write_ops=0: in that case a client will retry the very first operation sent to a tablet server in the context of a transaction until the tablet server completes the preliminary tasks mentioned above. The flag has runtime semantics, so no restart of a tablet server is required upon modification of the flag's setting. Each tablet replica maintains a txn_id --> TxnOpDispatcher map, with an entry's lifecycle as below: * An entry is added upon receiving write request in the context of a multi-row transaction. * An entry is removed upon applying either ParticipantOpPB::ABORT_TXN or ParticipantOpPB::FINALIZE_COMMIT operation. * If a write request is received after transaction has been committed or aborted, the entry is automatically removed once receiving corresponding error response from any of the following components: ** from TxnStatusManager in at attempts to register a participant in the context of committed/aborted transaction ** from the replica itself in an attempt to add ParticipantOpPB::BEGIN_COMMIT operation In other words, the system automatically gets rid of no-longer-needed TxnOpDispatcher entries. This patch also contains several test scenarios to cover the newly introduced functionality. I also updated other related tests to remove the artificial registration of corresponding transaction participants, so those tests now rely on the newly introduced functionality. Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 --- M src/kudu/client/batcher.cc M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/integration-tests/txn_commit-itest.cc A src/kudu/integration-tests/txn_write_ops-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 12 files changed, 2,317 insertions(+), 126 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/17037/10 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 10 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/ops/participant_op.cc File src/kudu/tablet/ops/participant_op.cc: http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/ops/participant_op.cc@197 PS9, Line 197: RETURN_NOT_OK(tablet_replica_->UnregisterTxnOpDispatcher(txn_id())); Apply() cannot fail, see L234 for the CHECK_OK(). I suspect we should be able to put this into the Prepare phase? -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 9 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 23 Feb 2021 23:15:38 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@602 PS9, Line 602: this->RegisteredParticipantCb(txn_id, s); > This gets set here and in the ParticipantTxnBeginCallback. Does that mean i Answering my own question, no I don't think it does. That said, we could probably do without the duplication. -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 9 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 23 Feb 2021 22:28:25 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/17037 )
Change subject: KUDU-2612 tablet servers automatically register txn participants
..
Patch Set 9:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/integration-tests/txn_write_ops-itest.cc
File src/kudu/integration-tests/txn_write_ops-itest.cc:
PS9:
Could you also add a test in which we delete a replica that has pending ops?
There are some code paths that probably aren't exercised in the dispatcher
around CheckRunning().
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/integration-tests/txn_write_ops-itest.cc@446
PS9, Line 446: GetSingleTxnOpDispatcher() {
nit: indent by 4 spaces
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.h
File src/kudu/tablet/tablet_replica.h:
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.h@443
PS9, Line 443: let
nit: give
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@602
PS9, Line 602: this->RegisteredParticipantCb(txn_id, s);
This gets set here and in the ParticipantTxnBeginCallback. Does that mean it
gets called twice?
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@607
PS9, Line 607: SetRegistrationScheduled
SetPreliminaryTasksScheduled()?
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@1179
PS9, Line 1179: TxnParticipantBegi
nit: BeginTxnParticipantOp? Same in the callback?
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@1260
PS9, Line 1260: preliminary_tasks_scheduled_
If the main purpose of this is to ensure we don't have multiple registrations
going at once, would it make sense to set 'preliminary_tasks_scheduled_' to
true here? Or maybe call it 'preliminary_tasks_being_scheduled_' or somesuch.
Would that help avoid the race you mention above?
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@1362
PS9, Line 1362: Cleanup
nit: maybe call this RespondFailures() or something to make it more obvious
that this should only be called on failure? Also, do we need to hold the lock
when checking the ops_queue_ and inflight_status_?
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tserver/ts_tablet_manager.cc@1166
PS9, Line 1166:
: if
(PREDICT_FALSE(FLAGS_txn_participant_begin_op_inject_latency_ms > 0)) {
: // This is to simulate a situation when txn participant
registration
: // above takes too long.
: SleepFor(MonoDelta::FromMilliseconds(
: FLAGS_txn_participant_begin_op_inject_latency_ms));
: }
nit: can use MAYBE_INJECT_FIXED_LATENCY(), same above.
http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tserver/ts_tablet_manager.cc@1180
PS9, Line 1180: d);
Would it make sense to pass 'cb' as an argument here to pass to
ParticipantTxnBeginCallback's constructor? AFAICT they're identical lambdas
anyway.
--
To view, visit http://gerrit.cloudera.org:8080/17037
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6
Gerrit-Change-Number: 17037
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Feb 2021 22:27:38 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 9: Code-Review+1 (1 comment) Not sure if the failed test is related? http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/tablet/tablet_replica.cc@1208 PS9, Line 1208: CancelPendingTxnOps Is it possible for CancelPendingTxnOps to be called twice if BEGIN_TXN op failed? Since RegisteredParticipantCb can also trigger CancelPendingTxnOps? What will the impact be if so? -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 9 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 23 Feb 2021 22:15:29 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/17037/9/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: PS9: I think it's better to move these new test scenarios into txn_write_ops-itest.cc: client-test.cc file is too crowded. -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 9 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 20 Feb 2021 06:52:09 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17037 to look at the new patch set (#9). Change subject: KUDU-2612 tablet servers automatically register txn participants .. KUDU-2612 tablet servers automatically register txn participants With this patch, tablet servers automatically register tablets as transaction participants and issue appropriate BEGIN_TXN operations upon receiving write operations targeting tablet replicas which they host. Internally, the newly introduced logic is mostly embedded into the TxnOpDispatcher class. Every TxnOpDispatcher object is allowed to accumulate up to a certain number of pending write requests in its queue while awaiting for the completion of the preliminary tasks mentioned above. Once the TxnOpDispatcher's queue is at capacity, a tablet server starts responding with the ErrorStatusPB::ERROR_SERVER_TOO_BUSY error code to incoming write requests in the context of the corresponding transaction, and Kudu clients automatically retry requests, as expected. The capacity of the TxnOpDispatcher's queue is controlled by a newly introduced --tablet_max_pending_txn_write_ops flag. By default, the flag is set to 2. Buffering a few write requests should help to avoid needless retries in case if a client packs many operations into one write request: that's exactly the behavior for Kudu client sessions using the AUTO_FLUSH_BACKGROUND mode. If such buffering isn't desired for some reason, set --tablet_max_pending_txn_write_ops=0: in that case a client will retry the very first operation sent to a tablet server in the context of a transaction until the tablet server completes the preliminary tasks mentioned above. The flag has runtime semantics, so no restart of a tablet server is required upon modification of the flag's setting. Each tablet replica maintains a txn_id --> TxnOpDispatcher map, with an entry's lifecycle as below: * An entry is added upon receiving write request in the context of a multi-row transaction. * An entry is removed upon applying either ParticipantOpPB::ABORT_TXN or ParticipantOpPB::FINALIZE_COMMIT operation. * If a write request is received after transaction has been committed or aborted, the entry is automatically removed once receiving corresponding error response from any of the following components: ** from TxnStatusManager in at attempts to register a participant in the context of committed/aborted transaction ** from the replica itself in an attempt to add ParticipantOpPB::BEGIN_COMMIT operation In other words, the system automatically gets rid of no-longer-needed TxnOpDispatcher entries. This patch also contains several test scenarios to cover the newly introduced functionality. I also updated other related tests to remove the artificial registration of corresponding transaction participants, so those tests now rely on the newly introduced functionality. Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 --- M src/kudu/client/batcher.cc M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/integration-tests/txn_commit-itest.cc A src/kudu/integration-tests/txn_write_ops-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 12 files changed, 2,035 insertions(+), 116 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/17037/9 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 9 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17037 to look at the new patch set (#8). Change subject: KUDU-2612 tablet servers automatically register txn participants .. KUDU-2612 tablet servers automatically register txn participants With this patch, tablet servers automatically register tablets as transaction participants and issue appropriate BEGIN_TXN operations upon receiving write operations targeting tablet replicas which they host. Internally, the newly introduced logic is mostly embedded into the TxnOpDispatcher class. Every TxnOpDispatcher object is allowed to accumulate up to a certain number of pending write requests in its queue while awaiting for the completion of the preliminary tasks mentioned above. Once the TxnOpDispatcher's queue is at capacity, a tablet server starts responding with the ErrorStatusPB::ERROR_SERVER_TOO_BUSY error code to incoming write requests in the context of the corresponding transaction, and Kudu clients automatically retry requests, as expected. The capacity of the TxnOpDispatcher's queue is controlled by a newly introduced --tablet_max_pending_txn_write_ops flag. By default, the flag is set to 2. Buffering a few write requests should help to avoid needless retries in case if a client packs many operations into one write request: that's exactly the behavior for Kudu client sessions using the AUTO_FLUSH_BACKGROUND mode. If such buffering isn't desired for some reason, set --tablet_max_pending_txn_write_ops=0: in that case a client will retry the very first operation sent to a tablet server in the context of a transaction until the tablet server completes the preliminary tasks mentioned above. The flag has runtime semantics, so no restart of a tablet server is required upon modification of the flag's setting. Each tablet replica maintains a txn_id --> TxnOpDispatcher map, with an entry's lifecycle as below: * An entry is added upon receiving write request in the context of a multi-row transaction. * An entry is removed upon applying either ParticipantOpPB::ABORT_TXN or ParticipantOpPB::FINALIZE_COMMIT operation. * If a write request is received after transaction has been committed or aborted, the entry is automatically removed once receiving corresponding error response from any of the following components: ** from TxnStatusManager in at attempts to register a participant in the context of committed/aborted transaction ** from the replica itself in an attempt to add ParticipantOpPB::BEGIN_COMMIT operation In other words, the system automatically gets rid of no-longer-needed TxnOpDispatcher entries. This patch also contains several test scenarios to cover the newly introduced functionality. I also updated other related tests to remove the artificial registration of corresponding transaction participants, so those tests now rely on the newly introduced functionality. Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 --- M src/kudu/client/batcher.cc M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/txn_commit-itest.cc A src/kudu/integration-tests/txn_write_ops-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 11 files changed, 2,031 insertions(+), 116 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/17037/8 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@1258 PS6, Line 1258: // Store the information necessary to recreate WriteOp instance: this is : // useful if TabletReplica::SubmitWrite() returns non-OK status. > Right, I guess more accurately I meant the contents of the request and resp Ah, I see. I didn't understand that your original comment was about the lifecycle of the underlying data for those pointers. Thank you for the clarification! Done. -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 20 Feb 2021 05:40:17 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@1258 PS6, Line 1258: : *need_participant_registration = > These pointers are stored at this level (i.e. scope of the while() cycle) a Right, I guess more accurately I meant the contents of the request and response. For instance if their lifecycle were tied to the lifecycle of op, the std::move(op) could result in the pointer copies pointing to bogus values. I guess, in question form, when does the cleanup for request happen? Iiuc it's only after the callback is invoked, so there's no risk of that cleanup interfering with the creation of a new op state. -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 20 Feb 2021 03:21:59 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 7: Verified+1 Overriding a couple of failures in RELEASE build: * ClientTest.TxnMultipleSingleRowsWithServerRestart is due to lower than needed timeout for write operation -- I'm planning to update that * BlockManagerType/TsRecoveryITest.TestCrashDuringLogReplay/0 -- unrelated test failure due to an attempt to start a webserver on already used port -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 20 Feb 2021 03:20:49 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has removed a vote on this change. Change subject: KUDU-2612 tablet servers automatically register txn participants .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/17037 )
Change subject: KUDU-2612 tablet servers automatically register txn participants
..
Patch Set 6:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/client/client-test.cc@7694
PS6, Line 7694: }
> Maybe count the rows to verify the state?
Done
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/ops/participant_op.cc
File src/kudu/tablet/ops/participant_op.cc:
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/ops/participant_op.cc@197
PS6, Line 197:
RETURN_NOT_OK(tablet_replica_->UnregisterTxnOpDispatcher(txn_id()));
> So begin commit can time out if the there are still writes pending when the
Right: if there are pending operations in the context of a particular
transaction, UnregisterTxnOpDispatcher() returns ServiceUnavailable(). It
seems reasonable to me: how it's possible to start committing a transaction
when there are still non-applied write operations?
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h
File src/kudu/tablet/tablet_replica.h:
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h@155
PS6, Line 155: txn_participant_registrant
> nit: doc this?
Done
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h@176
PS6, Line 176: void SubmitPendingTxnOps(int64_t txn_id);
: void CancelPendingTxnOps(int64_t txn_id, const Status& s);
> nit: would you mind doc these two? Thanks!
Done
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h@405
PS6, Line 405: //
> nit: spurious line
Done
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h@456
PS6, Line 456: ready_, ops_queue_.
> nit: if so, would be nice to at least mention how users should reason about
Done
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h@458
PS6, Line 458: bool ready_;
: bool unregistered_;
: Status inflight_status_;
: std::deque> ops_queue_;
> nit: would you mind also doc these?
Done
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@79
PS6, Line 79: 2
> how this is evaluated. If default is 2, it doesn't seem to help a lot for b
It should help: keep in mind that clients usually buffer inserts/updates to
many rows in one write request. Maybe, the name for the flag isn't quite good?
I can share more data once I have performance tests to cover this.
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@552
PS6, Line 552: dispatcher = FindOrNull(txn_op_dispatchers_, txn_id);
: if (!dispatcher) {
: dispatcher = (
: _op_dispatchers_,
: std::piecewise_construct,
: std::forward_as_tuple(txn_id),
: std::forward_as_tuple(this,
FLAGS_tablet_max_pending_txn_write_ops));
: }
> Does LookupOrEmplace work here?
Done
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@567
PS6, Line 567: SubmitTxnWriteCb
> nit: seems we typically name callbacks based on what we're calling back fro
Sure, let's keep it consistent.
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@590
PS6, Line 590: txn_op_dispatchers_.erase(it);
> Do we have any guarantees that we've submitted all the pending writes? Or a
Yes, we do: MarkUnregistered() would return non-OK status otherwise one line
above. In addition, TxnOpDispatcher doesn't accept any new operations once
it's marked as unregistered (see TxnOpDispatcher::Dispatch()).
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@642
PS6, Line 642: txn_registrator
> nit: dispatcher? Same elsewhere
Done
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@1227
PS6, Line 1227: transaction is not open"
> Don't we also unregister if we cancel due to not being the leader? Eg in Su
Right -- TxnOpDispatcher::Unregister() is called upon various errors, and not
being a leader (along with not being to register a participant) are the reasons.
I replaced it with "tablet replica could not accept txn write operation".
That's still vague, but I don't think we want to expose TxnOpDispatcher state
in the error messages.
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@1258
PS6, Line 1258: // Store the information necessary to recreate WriteOp
instance: this is
: // useful if TabletReplica::SubmitWrite() returns non-OK
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17037 to look at the new patch set (#7). Change subject: KUDU-2612 tablet servers automatically register txn participants .. KUDU-2612 tablet servers automatically register txn participants With this patch, tablet servers automatically register tablets as transaction participants and issue appropriate BEGIN_TXN operations upon receiving write operations targeting tablet replicas which they host. Internally, the newly introduced logic is mostly embedded into the TxnOpDispatcher class. Every TxnOpDispatcher object is allowed to accumulate up to a certain number of pending write requests in its queue while awaiting for the completion of the preliminary tasks mentioned above. Once the TxnOpDispatcher's queue is at capacity, a tablet server starts responding with the ErrorStatusPB::ERROR_SERVER_TOO_BUSY error code to incoming write requests in the context of the corresponding transaction, and Kudu clients automatically retry requests, as expected. The capacity of the TxnOpDispatcher's queue is controlled by a newly introduced --tablet_max_pending_txn_write_ops flag. By default, the flag is set to 2. Buffering a few write requests should help to avoid needless retries in case if a client packs many operations into one write request: that's exactly the behavior for Kudu client sessions using the AUTO_FLUSH_BACKGROUND mode. If such buffering isn't desired for some reason, set --tablet_max_pending_txn_write_ops=0: in that case a client will retry the very first operation sent to a tablet server in the context of a transaction until the tablet server completes the preliminary tasks mentioned above. The flag has runtime semantics, so no restart of a tablet server is required upon modification of the flag's setting. Each tablet replica maintains a txn_id --> TxnOpDispatcher map, with an entry's lifecycle as below: * An entry is added upon receiving write request in the context of a multi-row transaction. * An entry is removed upon applying either ParticipantOpPB::ABORT_TXN or ParticipantOpPB::FINALIZE_COMMIT operation. * If a write request is received after transaction has been committed or aborted, the entry is automatically removed once receiving corresponding error response from any of the following components: ** from TxnStatusManager in at attempts to register a participant in the context of committed/aborted transaction ** from the replica itself in an attempt to add ParticipantOpPB::BEGIN_COMMIT operation In other words, the system automatically gets rid of no-longer-needed TxnOpDispatcher entries. This patch also contains several test scenarios to cover the newly introduced functionality. I also updated other related tests to remove the artificial registration of corresponding transaction participants, so those tests now rely on the newly introduced functionality. Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 --- M src/kudu/client/batcher.cc M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/integration-tests/txn_commit-itest.cc A src/kudu/integration-tests/txn_write_ops-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 12 files changed, 2,045 insertions(+), 127 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/17037/7 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/17037 )
Change subject: KUDU-2612 tablet servers automatically register txn participants
..
Patch Set 6:
(7 comments)
Just a first pass through.
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/ops/participant_op.cc
File src/kudu/tablet/ops/participant_op.cc:
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/ops/participant_op.cc@197
PS6, Line 197:
RETURN_NOT_OK(tablet_replica_->UnregisterTxnOpDispatcher(txn_id()));
So begin commit can time out if the there are still writes pending when the
retries finish?
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h
File src/kudu/tablet/tablet_replica.h:
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h@176
PS6, Line 176: void SubmitPendingTxnOps(int64_t txn_id);
: void CancelPendingTxnOps(int64_t txn_id, const Status& s);
nit: would you mind doc these two? Thanks!
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h@458
PS6, Line 458: bool ready_;
: bool unregistered_;
: Status inflight_status_;
: std::deque> ops_queue_;
nit: would you mind also doc these?
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@79
PS6, Line 79: 2
how this is evaluated. If default is 2, it doesn't seem to help a lot for
buffering writes before begin txn?
http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/tserver/ts_tablet_manager.h
File src/kudu/tserver/ts_tablet_manager.h:
http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/tserver/ts_tablet_manager.h@247
PS5, Line 247: process
nit: to process for transactional write operations.
http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/tserver/ts_tablet_manager.h@273
PS5, Line 273: static void RegisterAndBeginParticipantTxnTask(
nit: add a comment.
http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/tserver/ts_tablet_manager.cc@1115
PS5, Line 1115: MonoTime deadline,
: StatusCallback cb) {
: VLOG(3) << Substitute("registering participant $0 for txn ID
$1",
: replica->tablet_id(), txn_id);
: DCHECK(txn_system_client);
: //
it is ok to check time out even before the actual RegisterParticipant get
called?
--
To view, visit http://gerrit.cloudera.org:8080/17037
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6
Gerrit-Change-Number: 17037
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 18 Feb 2021 21:49:50 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/17037 )
Change subject: KUDU-2612 tablet servers automatically register txn participants
..
Patch Set 6:
(12 comments)
Still need to digest the life cycle of the dispatcher and its callbacks, but
leaving some questions so far.
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/client/client-test.cc@7694
PS6, Line 7694: }
Maybe count the rows to verify the state?
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h
File src/kudu/tablet/tablet_replica.h:
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h@155
PS6, Line 155: txn_participant_registrant
nit: doc this?
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h@405
PS6, Line 405: //
nit: spurious line
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.h@456
PS6, Line 456: ready_, ops_queue_.
nit: if so, would be nice to at least mention how users should reason about
concurrency control for the other members
http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/tablet/tablet_replica.cc@1262
PS5, Line 1262: const bool has_request_id = op->has_request_id();
> Yep, that's done for all pending write operations in the queue. Here the q
If we've failed, and updated Init() such that failure calls the client
callback, we would have already responded to the client and wouldn't need to
add it to the queue.
In any case, I think your new approach is good and less intrusive. I was at
first worried that the response could be garbage after the std::move() but
seems it's only freed after the callback is called.
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@552
PS6, Line 552: dispatcher = FindOrNull(txn_op_dispatchers_, txn_id);
: if (!dispatcher) {
: dispatcher = (
: _op_dispatchers_,
: std::piecewise_construct,
: std::forward_as_tuple(txn_id),
: std::forward_as_tuple(this,
FLAGS_tablet_max_pending_txn_write_ops));
: }
Does LookupOrEmplace work here?
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@567
PS6, Line 567: SubmitTxnWriteCb
nit: seems we typically name callbacks based on what we're calling back from,
rather than what the callback does. Maybe call this RegisteredParticipantCb()
or somesuch?
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@590
PS6, Line 590: txn_op_dispatchers_.erase(it);
Do we have any guarantees that we've submitted all the pending writes? Or at
least, called all the callbacks?
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@642
PS6, Line 642: txn_registrator
nit: dispatcher? Same elsewhere
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@1227
PS6, Line 1227: transaction is not open"
Don't we also unregister if we cancel due to not being the leader? Eg in
SubmitTxnParticipantBeginOp() we may fail to submit the participant op?
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@1258
PS6, Line 1258: // Store the information necessary to recreate WriteOp
instance: this is
: // useful if TabletReplica::SubmitWrite() returns non-OK
status.
nit: it'd be nice if you could mention the lifecycle of the request and
response pointers. At first glance, it seems like these values could all be
bogus upon the std::move() call, but I don't think that's the case.
http://gerrit.cloudera.org:8080/#/c/17037/6/src/kudu/tablet/tablet_replica.cc@1304
PS6, Line 1304: Cleanup
What's the point of returning a status here? Doesn't this always return
'status'?
--
To view, visit http://gerrit.cloudera.org:8080/17037
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6
Gerrit-Change-Number: 17037
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 17 Feb 2021 21:12:01 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 6: > unrelated build/test failures: > * DEBUG: strange issue with freeing gflags memory -- SIGSEGV in > libtcmalloc > * RELEASE: build failure due to python dependency issues (now fixed > with f24ec6a7757af421f6da3e5f4b93f3a6761d23bb) > * TSAN: a race between ScheduleTxnManagerInit() and > Master::ShutdownImpl() The latter race is addressed with http://gerrit.cloudera.org:8080/17076 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 17 Feb 2021 19:35:27 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 6: Verified+1 unrelated build/test failures: * DEBUG: strange issue with freeing gflags memory -- SIGSEGV in libtcmalloc * RELEASE: build failure due to python dependency issues (now fixed with f24ec6a7757af421f6da3e5f4b93f3a6761d23bb) * TSAN: a race between ScheduleTxnManagerInit() and Master::ShutdownImpl() -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 17 Feb 2021 18:47:39 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has removed a vote on this change. Change subject: KUDU-2612 tablet servers automatically register txn participants .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/tablet/tablet_replica.cc@1262 PS5, Line 1262: auto s = replica_->SubmitWrite(std::move(op)); > One approach that comes to mind is to have OpDriver::Init() run the op comp Yep, that's done for all pending write operations in the queue. Here the question is how to put the failed op into the queue, since the ownership for that op is transferred down the control path. I considered two options here, and update the code here with one of those: see PS6. -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 17 Feb 2021 07:33:19 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17037 to look at the new patch set (#6). Change subject: KUDU-2612 tablet servers automatically register txn participants .. KUDU-2612 tablet servers automatically register txn participants With this patch, tablet servers automatically register tablets as transaction participants and automatically issue BEGIN_TXN operation for the replica upon receiving write operations targeting tablet replicas which they host. Internally, the newly introduced logic is mostly embedded into a new TxnOpDispatcher class. Every tablet replica maintains a txn_id --> TxnOpDispatcher map, and here is the lifecycle of an entry in that map: * An entry is added upon receiving write request in the context of a multi-row transaction. * An entry is removed upon applying either ParticipantOpPB::ABORT_TXN or ParticipantOpPB::FINALIZE_COMMIT operation. * If a write request is received after transaction has been committed or aborted, the entry is automatically removed once receiving corresponding error response from any of the following components: ** from TxnStatusManager in at attempts to register a participant in the context of committed/aborted transaction ** from the replica itself in an attempt to add ParticipantOpPB::BEGIN_COMMIT operation In other words, the system automatically gets rid of no-longer-needed TxnOpDispatcher entries. This patch also contains several test scenarios to cover the newly introduced functionality. I also updated other related tests to remove artificial registration of corresponding transaction participants, relying on newly introduced automatic registration of those. WIP-ish: * add more tests Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 --- M src/kudu/client/batcher.cc M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/integration-tests/txn_commit-itest.cc A src/kudu/integration-tests/txn_write_ops-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 12 files changed, 1,558 insertions(+), 127 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/17037/6 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/17037 )
Change subject: KUDU-2612 tablet servers automatically register txn participants
..
Patch Set 2:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/integration-tests/fuzz-itest.cc@245
PS3, Line 245: val(std::move(v)) {}
> warning: std::move of the variable 'v' of the trivially-copyable type 'opti
ignored
http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/integration-tests/txn_write_ops-itest.cc
File src/kudu/integration-tests/txn_write_ops-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/integration-tests/txn_write_ops-itest.cc@90
PS3, Line 90:
> warning: using decl 'TabletServer' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/integration-tests/txn_write_ops-itest.cc@185
PS3, Line 185:
> warning: std::move of the const expression has no effect; remove std::move(
Done
http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tablet/tablet_replica.h
File src/kudu/tablet/tablet_replica.h:
http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tablet/tablet_replica.h@144
PS3, Line 144: // TODO have a way to wait for any state?
> warning: missing username/bug in TODO [google-readability-todo]
Done
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.h
File src/kudu/tablet/tablet_replica.h:
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.h@371
PS2, Line 371: DoIt
> Heh :)
Yeah, this patch is very raw: I had too much distractions in the process, sorry
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.h@485
PS2, Line 485: std::unordered_map
txn_registrators_;
> Will this ever get erased from? Maybe when we apply the commit or abort for
Done
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.cc@1194
PS2, Line 1194: TxnParticipantRegistrator
> nit: maybe TxnParticipantRegistrant?
I ended up renaming it into TxnOpDispatcher -- the rationale is that this class
dispatches all txn-enabled write operations processed by a tablet replica.
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.cc@1234
PS2, Line 1234: // If there are still pending operations into the queue,
enqueue this one
: // to order them in FIFO manner.
> Would it make sense to submit them all here?
Well, it turns out that's not possible to have 'ready_' set to 'true' and have
non-empty 'ops_queue_'. I added corresponding CHECK().
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.cc@1258
PS2, Line 1258: std::lock_guard guard(lock_);
: while (!ops_queue_.empty()) {
> Is it possible that we'll insert to the queue as we're submitting? or after
Right -- that's a point to clarify (and, actually, re-do this).
http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tablet/tablet_replica.cc@540
PS3, Line 540: RETURN_NOT_OK(CheckRunning());
> warning: the parameter 'txn_participant_registrant' is copied for each invo
Done
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/transactions/txn_system_client.cc
File src/kudu/transactions/txn_system_client.cc:
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/transactions/txn_system_client.cc@422
PS2, Line 422: // TODO(aserbin): should it be done only conditionally?
: s = txn_client->OpenTxnStatusTable();
: }
> Might this cause issues if lazily initializing the transaction status table
Exactly -- I was weighing in a couple of alternatives: I moved this out from
here and switched to TxnSystemClient::CheckOpenTxnSystemTable()
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tserver/tablet_service.cc@1655
PS2, Line 1655: avoid dependency on TxnSystemClient.
> I'm fine leaving this as is, but another approach would be to use generics
I resorted to using std::function instead: I wasn't sure introducing a factory
was the best option. I can be convinced otherwise, of course :)
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tserver/ts_tablet_manager.cc@1510
PS2, Line 1510: RegisterParticipant
> A future improvement we can do here is to use the async variant that shares
Good point! I slightly modified
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 ) Change subject: KUDU-2612 tablet servers automatically register txn participants .. Patch Set 5: (2 comments) Not a full review, just mentioning this since this issue seems a tricky. http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/integration-tests/txn_write_ops-itest.cc File src/kudu/integration-tests/txn_write_ops-itest.cc: http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/integration-tests/txn_write_ops-itest.cc@178 PS5, Line 178: kNumTablets Should this be kNumTablets + 1, for the TxnStatusTablet? http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/5/src/kudu/tablet/tablet_replica.cc@1262 PS5, Line 1262: auto s = replica_->SubmitWrite(std::move(op)); One approach that comes to mind is to have OpDriver::Init() run the op completion callback on failure if we're the leader, since AFAICT that's the main error codepath exercised by SubmitWrite(). -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 17 Feb 2021 00:45:18 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17037 to look at the new patch set (#5). Change subject: KUDU-2612 tablet servers automatically register txn participants .. KUDU-2612 tablet servers automatically register txn participants With this patch, tablet servers automatically register tablets as transaction participants and automatically issue BEGIN_TXN operation for the replica upon receiving write operations targeting tablet replicas they host. This patch also contains several test scenarios to cover the newly introduced functionality. Here is an TxnOpDispatcher entry lifecycle: * An entry is added upon receiving write request in the context of a multi-row transaction. * An entry is removed upon applying either ParticipantOpPB::ABORT_TXN or ParticipantOpPB::FINALIZE_COMMIT operation. * If a write request is received after transaction has been committed or aborted, the entry is automatically removed once receiving corresponding error response from any of the following components: ** from TxnStatusManager in at attempts to register a participant in the context of committed/aborted transaction ** from the replica itself in an attempt to add ParticipantOpPB::BEGIN_COMMIT operation In other words, TxnOpDispatcher entries are automatically cleaned up and do not require any extra task. WIP-ish: * address TODO to TxnOpDispatcher::Submit() addressing ownership transfer on WriteOpState back to the code when TabletReplica::SubmitWrite() returns non-OK status * add more tests Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 --- M src/kudu/client/batcher.cc M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/integration-tests/txn_commit-itest.cc A src/kudu/integration-tests/txn_write_ops-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 12 files changed, 1,546 insertions(+), 124 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/17037/5 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 tablet servers automatically register txn participants
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17037 to look at the new patch set (#4). Change subject: KUDU-2612 tablet servers automatically register txn participants .. KUDU-2612 tablet servers automatically register txn participants With this patch, tablet servers automatically register tablets as transaction participants and automatically issue BEGIN_TXN operation for the replica upon receiving write operations targeting tablet replicas they host. This patch also contains several test scenarios to cover the newly introduced functionality. Here is an TxnOpDispatcher entry lifecycle: * An entry is added upon receiving write request in the context of a multi-row transaction. * An entry is removed upon applying either ParticipantOpPB::ABORT_TXN or ParticipantOpPB::FINALIZE_COMMIT operation. * If a write request is received after transaction has been committed or aborted, the entry is automatically removed once receiving corresponding error response from any of the following components: ** from TxnStatusManager in at attempts to register a participant in the context of committed/aborted transaction ** from the replica itself in an attempt to add ParticipantOpPB::BEGIN_COMMIT operation In other words, TxnOpDispatcher entries are automatically cleaned up and do not require any extra task. WIP-ish: * address TODO to TxnOpDispatcher::Submit() addressing ownership transfer on WriteOpState back to the code when TabletReplica::SubmitWrite() returns non-OK status * add more tests Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 --- M src/kudu/client/batcher.cc M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/integration-tests/txn_commit-itest.cc A src/kudu/integration-tests/txn_write_ops-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 12 files changed, 1,548 insertions(+), 126 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/17037/4 -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
