You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2021/02/03 02:27:32 UTC
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17017
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
KUDU-2612: add background task to abort transaction participants
This patch implements background tasks that abort a given transaction
when TxnStatusManager::AbortTransaction(). Similar to the commit tasks,
aborts have the following life cycle:
1. AbortTransaction() is called. A new state, ABORT_IN_PROGRESS, is
written to the TxnStatusManager.
2. ABORT_TXN ops are sent to all participants in the transaction.
3. Once all participants have responded, the ABORTED state is written
to the TxnStatusManager.
Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
---
M src/kudu/transactions/transactions.proto
M src/kudu/transactions/txn_status_manager.cc
M src/kudu/transactions/txn_status_manager.h
3 files changed, 155 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/17017/1
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17017
to look at the new patch set (#6).
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
KUDU-2612: add background task to abort transaction participants
This patch implements background tasks that abort a given transaction
when TxnStatusManager::AbortTransaction() is called. Similar to the
commit tasks, aborts have the following life cycle:
1. AbortTransaction() is called. A new state, ABORT_IN_PROGRESS, is
written to the TxnStatusManager.
2. ABORT_TXN ops are sent to all participants in the transaction.
3. Once all participants have responded, the ABORTED state is written
to the TxnStatusManager.
This patch doesn't test races between commits and aborts. Some reworking
of the commit tasks will be required to account for such races, and will
be done in a follow-up.
Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java
M src/kudu/client/client-test.cc
M src/kudu/client/transaction-internal.cc
M src/kudu/integration-tests/txn_commit-itest.cc
M src/kudu/integration-tests/txn_status_manager-itest.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/txn_manager-test.cc
M src/kudu/transactions/transactions.proto
M src/kudu/transactions/txn_status_manager-test.cc
M src/kudu/transactions/txn_status_manager.cc
M src/kudu/transactions/txn_status_manager.h
11 files changed, 416 insertions(+), 60 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/17017/6
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17017
to look at the new patch set (#5).
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
KUDU-2612: add background task to abort transaction participants
This patch implements background tasks that abort a given transaction
when TxnStatusManager::AbortTransaction(). Similar to the commit tasks,
aborts have the following life cycle:
1. AbortTransaction() is called. A new state, ABORT_IN_PROGRESS, is
written to the TxnStatusManager.
2. ABORT_TXN ops are sent to all participants in the transaction.
3. Once all participants have responded, the ABORTED state is written
to the TxnStatusManager.
This patch doesn't test races between commits and aborts. Some reworking
of the commit tasks will be required to account for such races, and will
be done in a follow-up.
Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java
M src/kudu/client/transaction-internal.cc
M src/kudu/integration-tests/txn_commit-itest.cc
M src/kudu/integration-tests/txn_status_manager-itest.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/txn_manager-test.cc
M src/kudu/transactions/transactions.proto
M src/kudu/transactions/txn_status_manager-test.cc
M src/kudu/transactions/txn_status_manager.cc
M src/kudu/transactions/txn_status_manager.h
10 files changed, 356 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/17017/5
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17017 )
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
Patch Set 8: Verified+1
Test failure is unrelated.
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 09 Feb 2021 21:40:02 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17017 )
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
Patch Set 8: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc
File src/kudu/integration-tests/txn_commit-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@332
PS5, Line 332: UALLY([&] {
> At first my rationale was that if the transaction was aborted, from a clien
Ack, sounds good to me as well.
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/transactions/txn_status_manager.cc@982
PS5, Line 982: RETURN_NOT_OK(status_tablet_.UpdateTransaction(
: txn_id, mutable_data->pb, ts_error));
: txn_lock.Commit();
: return Status::OK();
: }
:
: Status TxnStatusManager::FinalizeAbortTransaction(int64_t txn_id) {
: leader_lock_.AssertAcquiredForReading();
:
> Ah nice catch! This isn't necessary -- the commit tasks clean up after them
Ack
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 10 Feb 2021 00:37:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17017 )
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/client/transaction-internal.cc
File src/kudu/client/transaction-internal.cc:
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/client/transaction-internal.cc@309
PS5, Line 309: case TxnStatePB::ABORT_IN_PROGRESS:
: case TxnStatePB::ABORTED:
: *is_complete = true;
: *completion_status = Status::Aborted("transaction has been aborted");
I understand we consider ABORT_IN_PROGRESS to be eventually aborted no matter what. But do you think it still worth returning different error msg to differentiate txn that is actually aborted v.s. still aborting?
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/transactions/txn_status_manager.cc@331
PS5, Line 331: write the finalized
: // commit timestamp to the tablet.
nit: update to write the abort record?
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/transactions/txn_status_manager.cc@982
PS5, Line 982: mutable_data->pb.set_commit_timestamp(commit_timestamp.value());
: RETURN_NOT_OK(status_tablet_.UpdateTransaction(
: txn_id, mutable_data->pb, ts_error));
:
: {
: std::lock_guard<simple_spinlock> l(lock_);
: commits_in_flight_.erase(txn_id);
: }
:
Just curious, it seems this should belong to the previous patch which introduced background task to commit transactions?
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 09 Feb 2021 06:50:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17017
to look at the new patch set (#7).
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
KUDU-2612: add background task to abort transaction participants
This patch implements background tasks that abort a given transaction
when TxnStatusManager::AbortTransaction() is called. Similar to the
commit tasks, aborts have the following life cycle:
1. AbortTransaction() is called. A new state, ABORT_IN_PROGRESS, is
written to the TxnStatusManager.
2. ABORT_TXN ops are sent to all participants in the transaction.
3. Once all participants have responded, the ABORTED state is written
to the TxnStatusManager.
This patch doesn't test races between commits and aborts. Some reworking
of the commit tasks will be required to account for such races, and will
be done in a follow-up.
Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java
M src/kudu/client/client-test.cc
M src/kudu/client/transaction-internal.cc
M src/kudu/integration-tests/txn_commit-itest.cc
M src/kudu/integration-tests/txn_status_manager-itest.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/txn_manager-test.cc
M src/kudu/transactions/transactions.proto
M src/kudu/transactions/txn_status_manager-test.cc
M src/kudu/transactions/txn_status_manager.cc
M src/kudu/transactions/txn_status_manager.h
12 files changed, 422 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/17017/7
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17017
to look at the new patch set (#3).
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
KUDU-2612: add background task to abort transaction participants
This patch implements background tasks that abort a given transaction
when TxnStatusManager::AbortTransaction(). Similar to the commit tasks,
aborts have the following life cycle:
1. AbortTransaction() is called. A new state, ABORT_IN_PROGRESS, is
written to the TxnStatusManager.
2. ABORT_TXN ops are sent to all participants in the transaction.
3. Once all participants have responded, the ABORTED state is written
to the TxnStatusManager.
This patch doesn't test races between commits and aborts. Some reworking
of the commit tasks will be required to account for such races, and will
be done in a follow-up.
Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
---
M src/kudu/client/transaction-internal.cc
M src/kudu/integration-tests/txn_commit-itest.cc
M src/kudu/integration-tests/txn_status_manager-itest.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/txn_manager-test.cc
M src/kudu/transactions/transactions.proto
M src/kudu/transactions/txn_status_manager-test.cc
M src/kudu/transactions/txn_status_manager.cc
M src/kudu/transactions/txn_status_manager.h
9 files changed, 350 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/17017/3
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17017 )
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
KUDU-2612: add background task to abort transaction participants
This patch implements background tasks that abort a given transaction
when TxnStatusManager::AbortTransaction() is called. Similar to the
commit tasks, aborts have the following life cycle:
1. AbortTransaction() is called. A new state, ABORT_IN_PROGRESS, is
written to the TxnStatusManager.
2. ABORT_TXN ops are sent to all participants in the transaction.
3. Once all participants have responded, the ABORTED state is written
to the TxnStatusManager.
This patch doesn't test races between commits and aborts. Some reworking
of the commit tasks will be required to account for such races, and will
be done in a follow-up.
Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Reviewed-on: http://gerrit.cloudera.org:8080/17017
Tested-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Hao Hao <ha...@cloudera.com>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java
M src/kudu/client/client-test.cc
M src/kudu/client/transaction-internal.cc
M src/kudu/integration-tests/txn_commit-itest.cc
M src/kudu/integration-tests/txn_status_manager-itest.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/txn_manager-test.cc
M src/kudu/transactions/transactions.proto
M src/kudu/transactions/txn_status_manager-test.cc
M src/kudu/transactions/txn_status_manager.cc
M src/kudu/transactions/txn_status_manager.h
12 files changed, 422 insertions(+), 61 deletions(-)
Approvals:
Andrew Wong: Verified
Hao Hao: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17017 )
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
Patch Set 6:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/17017/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17017/5//COMMIT_MSG@10
PS5, Line 10: when TxnStatusManager::AbortTransaction()
> nit: when TxnStatusManager::AbortTransaction() is called
Done
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/client/transaction-internal.cc
File src/kudu/client/transaction-internal.cc:
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/client/transaction-internal.cc@309
PS5, Line 309: case TxnStatePB::ABORT_IN_PROGRESS:
: *is_complete = false;
: *completion_status = Status::Aborted("transaction is being aborted");
: break;
> I understand we consider ABORT_IN_PROGRESS to be eventually aborted no matt
Done
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc
File src/kudu/integration-tests/txn_commit-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@262
PS5, Line 262:
> nit: if you interested in syntactic sugar by any chance, with C++17 it's po
Nice! That's really convenient, thanks! I separated out a few of these into a separate patch.
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@330
PS5, Line 330: // to wait for this to happen, since 'is_complete' is contingent on
> So, this now reports IsAborted() for both TXN_ABORTED and TXN_ABORT_IN_PROG
Right. Though I updated this so 'is_complete' is only true if we've finished aborting.
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@332
PS5, Line 332: UALLY([&] {
> What should we report on a status of a transaction in ABORT_IN_PROGRESS sta
At first my rationale was that if the transaction was aborted, from a client perspective, there is no difference between ABORT_IN_PROGRESS and ABORTED, since any input rows wouldnt be returned either way. After thinking about it a bit more though, there is a difference in that ABORT_IN_PROGRESS implies that locks may still be held, whereas ABORTED implies that all locks have been released, which does seem to be useful to know about. I'll take this suggestion.
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@784
PS5, Line 784:
> nit: finalize commit and abort
Done
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/transactions/txn_status_manager.cc@331
PS5, Line 331:
: }
> nit: update to write the abort record?
Done
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/transactions/txn_status_manager.cc@982
PS5, Line 982: RETURN_NOT_OK(status_tablet_.UpdateTransaction(
: txn_id, mutable_data->pb, ts_error));
: txn_lock.Commit();
: return Status::OK();
: }
:
: Status TxnStatusManager::FinalizeAbortTransaction(int64_t txn_id) {
: leader_lock_.AssertAcquiredForReading();
:
> Just curious, it seems this should belong to the previous patch which intro
Ah nice catch! This isn't necessary -- the commit tasks clean up after themselves. This must be from an earlier version based off an older version of the previous patch.
As for the commit timestamp, I'll remove it from this patch, but re-add in in a follow-up, since I need to update the commit logic a bit to handle races between commit and abort.
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 09 Feb 2021 07:18:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17017
to look at the new patch set (#4).
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
KUDU-2612: add background task to abort transaction participants
This patch implements background tasks that abort a given transaction
when TxnStatusManager::AbortTransaction(). Similar to the commit tasks,
aborts have the following life cycle:
1. AbortTransaction() is called. A new state, ABORT_IN_PROGRESS, is
written to the TxnStatusManager.
2. ABORT_TXN ops are sent to all participants in the transaction.
3. Once all participants have responded, the ABORTED state is written
to the TxnStatusManager.
This patch doesn't test races between commits and aborts. Some reworking
of the commit tasks will be required to account for such races, and will
be done in a follow-up.
Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTransaction.java
M src/kudu/client/transaction-internal.cc
M src/kudu/integration-tests/txn_commit-itest.cc
M src/kudu/integration-tests/txn_status_manager-itest.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/txn_manager-test.cc
M src/kudu/transactions/transactions.proto
M src/kudu/transactions/txn_status_manager-test.cc
M src/kudu/transactions/txn_status_manager.cc
M src/kudu/transactions/txn_status_manager.h
10 files changed, 356 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/17017/4
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17017
to look at the new patch set (#2).
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
KUDU-2612: add background task to abort transaction participants
This patch implements background tasks that abort a given transaction
when TxnStatusManager::AbortTransaction(). Similar to the commit tasks,
aborts have the following life cycle:
1. AbortTransaction() is called. A new state, ABORT_IN_PROGRESS, is
written to the TxnStatusManager.
2. ABORT_TXN ops are sent to all participants in the transaction.
3. Once all participants have responded, the ABORTED state is written
to the TxnStatusManager.
This patch doesn't test races between commits and aborts. Some reworking
of the commit tasks will be required to account for such races, and will
be done in a follow-up.
Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
---
M src/kudu/client/transaction-internal.cc
M src/kudu/integration-tests/txn_commit-itest.cc
M src/kudu/integration-tests/txn_status_manager-itest.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/txn_manager-test.cc
M src/kudu/transactions/transactions.proto
M src/kudu/transactions/txn_status_manager-test.cc
M src/kudu/transactions/txn_status_manager.cc
M src/kudu/transactions/txn_status_manager.h
9 files changed, 252 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/17017/2
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612: add background task to abort transaction participants
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17017 )
Change subject: KUDU-2612: add background task to abort transaction participants
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/17017/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17017/5//COMMIT_MSG@10
PS5, Line 10: when TxnStatusManager::AbortTransaction()
nit: when TxnStatusManager::AbortTransaction() is called
?
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc
File src/kudu/integration-tests/txn_commit-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@262
PS5, Line 262: (const auto& txn_id_and_count : aborts_per_txn) {
nit: if you interested in syntactic sugar by any chance, with C++17 it's possible to write this as
for (const auto& [txn_id, count] : aborts_per_txn) {
...
}
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@330
PS5, Line 330: ASSERT_OK(txn->IsCommitComplete(&is_complete, &completion_status));
So, this now reports IsAborted() for both TXN_ABORTED and TXN_ABORT_IN_PROGRESS, right?
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@332
PS5, Line 332: is_complete
What should we report on a status of a transaction in ABORT_IN_PROGRESS state? Should IsCommitComplete():
* return 'false' for 'is_complete' if txn is in TXN_ABORT_IN_PROGRESS
* return 'true' for 'is_complete' if txn is in ABORTED
?
http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@784
PS5, Line 784: commit
nit: finalize commit and abort
?
--
To view, visit http://gerrit.cloudera.org:8080/17017
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e
Gerrit-Change-Number: 17017
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 08 Feb 2021 22:55:28 +0000
Gerrit-HasComments: Yes