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 2020/06/07 00:42:56 UTC
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16044
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
KUDU-2612 p2: introduce transaction status management
This introduces the TxnStatusManager that is backed by the
TxnStatusTablet that exposes the following APIs:
- BeginTransaction
- BeginCommitTransaction
- FinalizeCommitTransaction
- AbortTransaction
- RegisterParticipant
These are many of the building blocks for orchestrating two-phase
commit (though notably missing are commit/abort for participants, which
will come later).
This is at least enough of a jumping off point that we can begin
plumbing this into the tablet servers and defining an RPC service around
it.
Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
---
M src/kudu/master/catalog_manager.h
M src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/status_entry.cc
A src/kudu/transactions/status_entry.h
A src/kudu/transactions/status_manager-test.cc
A src/kudu/transactions/status_manager.cc
A src/kudu/transactions/status_manager.h
M src/kudu/util/cow_object.h
8 files changed, 1,029 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/1
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@328
PS6, Line 328:
> I still see LOG(DFATAL) instead of FAIL() here. It seems the usage of thes
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404
PS6, Line 404: _TRUE
> IIRC, in writing it's more common to avoid the contraction and use "cannot"
That's true, though I think we tend to have a somewhat conversational tone in comments, rather than formal. I typically reserve such formality for written essays, whitepapers, newspapers, etc.
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 21:22:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 9: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16044/9/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/9/src/kudu/transactions/txn_status_manager-test.cc@356
PS9, Line 356: s = txn_manager_->RegisterParticipant(1, ParticipantId(1), kWrongUser);
Whoops, I missed this last review round: the result status is not checked.
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 21:44:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16044
to look at the new patch set (#7).
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
KUDU-2612 p2: introduce transaction status management
This introduces the TxnStatusManager, which is backed by the
TxnStatusTablet that exposes the following APIs that will be called via
RPC, and will serve as many of the building blocks for orchestrating
two-phase commit:
- BeginTransaction: adds a new transaction under management of the
TxnStatusManager
- BeginCommitTransaction: transitions the state of a transaction from
OPEN to COMMIT_IN_PROGRESS
- AbortTransaction: transitions the state of a transaction from OPEN or
COMMIT_IN_PROGRESS to ABORTED
- RegisterParticipant: adds a participant to be associated with a
specific transaction ID
For completeness sake w.r.t defining the transaction state's enums, the
following API is also added, which will be called by the
TxnStatusManager itself upon determining a transaction has been
completed.
- FinalizeCommitTransaction: transitions the state of a transaction from
COMMIT_IN_PROGRESS to COMMITTED
This new abstraction mirrors that used by the CatalogManager, which uses
copy-on-write locking to protect concurrent access to metadata while
writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this
case, TxnStatusTablet) are being replicated.
This is at least enough of a jumping off point that we can begin
plumbing this into the tablet servers and defining an RPC service around
it -- there are still no facilities to create a TxnStatusManager.
It should be noted that end-users will not call these methods directly,
but rather through some layer of indirection (e.g. clients won't request
a specific transaction ID, they'll just request to begin a transaction,
and some intermediary layer will be in charge of getting an appropriate
transaction ID). This should give us flexibility in changing the
TxnStatusManager's interface moving forward.
Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
---
M src/kudu/master/catalog_manager.h
M src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/txn_status_entry.cc
A src/kudu/transactions/txn_status_entry.h
A src/kudu/transactions/txn_status_manager-test.cc
A src/kudu/transactions/txn_status_manager.cc
A src/kudu/transactions/txn_status_manager.h
M src/kudu/util/cow_object.h
8 files changed, 1,074 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/7
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16044
to look at the new patch set (#5).
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
KUDU-2612 p2: introduce transaction status management
This introduces the TxnStatusManager, which is backed by the
TxnStatusTablet that exposes the following APIs that will be called via
RPC, and will serve as many of the building blocks for orchestrating
two-phase commit:
- BeginTransaction: adds a new transaction under management of the
TxnStatusManager
- BeginCommitTransaction: transitions the state of a transaction from
OPEN to COMMIT_IN_PROGRESS
- AbortTransaction: transitions the state of a transaction from OPEN or
COMMIT_IN_PROGRESS to ABORTED
- RegisterParticipant: adds a participant to be associated with a
specific transaction ID
For completeness sake w.r.t defining the transaction state's enums, the
following API is also added, which will be called by the
TxnStatusManager itself upon determining a transaction has been
completed.
- FinalizeCommitTransaction: transitions the state of a transaction from
COMMIT_IN_PROGRESS to COMMITTED
This new abstraction mirrors that used by the CatalogManager, which uses
copy-on-write locking to protect concurrent access to metadata while
writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this
case, TxnStatusTablet) are being replicated.
This is at least enough of a jumping off point that we can begin
plumbing this into the tablet servers and defining an RPC service around
it -- there are still no facilities to create a TxnStatusManager.
Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
---
M src/kudu/master/catalog_manager.h
M src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/txn_status_entry.cc
A src/kudu/transactions/txn_status_entry.h
A src/kudu/transactions/txn_status_manager-test.cc
A src/kudu/transactions/txn_status_manager.cc
A src/kudu/transactions/txn_status_manager.h
M src/kudu/util/cow_object.h
8 files changed, 1,047 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/5
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16044/10/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/10/src/kudu/transactions/txn_status_manager-test.cc@326
PS10, Line 326: AIL() << "bad up
Oops, seems I clobbered over this change.
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 22:44:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 5:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_entry.h
File src/kudu/transactions/txn_status_entry.h:
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_entry.h@43
PS5, Line 43: status
transaction?
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@176
PS5, Line 176: make_shared
shouldn't these be unique_ptrs?
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@204
PS5, Line 204: // As a sanity check, there should have been at least one success per batch.
I think it helps understanding this if you add that the reason why not all of the transactions are successful is that the BeginTransaction will return an error if it knows about a higher txn id.
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@236
PS5, Line 236: // have registered participants before the transaction was registered.
why not simply registering the txn first and then registering only the participants concurrently and expect all of them to succeed?
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@304
PS5, Line 304: transactioN
nit: lower-case "n"
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h
File src/kudu/transactions/txn_status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@55
PS5, Line 55: void VisitTransactionEntries(int64_t txn_id, TxnStatusEntryPB status_entry_pb,
shouldn't these methods be commented?
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@73
PS5, Line 73: an
nit: a
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@77
PS5, Line 77: Status BeginTransaction(int64_t txn_id, const std::string& user);
Wouldn't it be easier to change txn_id to an out parameter that is automatically incremented? otherwise multiple clients could try to begin a transaction with the same ID and have to retry.
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@86
PS5, Line 86: is user-initiated
nit: isn't?
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 30 Jun 2020 16:54:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h
File src/kudu/transactions/txn_status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@77
PS5, Line 77: Status LoadFromTablet();
> We discussed this with Andrew a bit last week or so. As I understand, it's
Thanks for summarizing the discussion Alexey! I amended the TODO to be more inclusive of other paths forward, and I updated the design doc to mention them a bit as well.
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 17:39:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16044
to look at the new patch set (#8).
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
KUDU-2612 p2: introduce transaction status management
This introduces the TxnStatusManager, which is backed by the
TxnStatusTablet that exposes the following APIs that will be called via
RPC, and will serve as many of the building blocks for orchestrating
two-phase commit:
- BeginTransaction: adds a new transaction under management of the
TxnStatusManager
- BeginCommitTransaction: transitions the state of a transaction from
OPEN to COMMIT_IN_PROGRESS
- AbortTransaction: transitions the state of a transaction from OPEN or
COMMIT_IN_PROGRESS to ABORTED
- RegisterParticipant: adds a participant to be associated with a
specific transaction ID
For completeness sake w.r.t defining the transaction state's enums, the
following API is also added, which will be called by the
TxnStatusManager itself upon determining a transaction has been
completed.
- FinalizeCommitTransaction: transitions the state of a transaction from
COMMIT_IN_PROGRESS to COMMITTED
This new abstraction mirrors that used by the CatalogManager, which uses
copy-on-write locking to protect concurrent access to metadata while
writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this
case, TxnStatusTablet) are being replicated.
This is at least enough of a jumping off point that we can begin
plumbing this into the tablet servers and defining an RPC service around
it -- there are still no facilities to create a TxnStatusManager.
It should be noted that end-users will not call these methods directly,
but rather through some layer of indirection (e.g. clients won't request
a specific transaction ID, they'll just request to begin a transaction,
and some intermediary layer will be in charge of getting an appropriate
transaction ID). This should give us flexibility in changing the
TxnStatusManager's interface moving forward.
Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
---
M src/kudu/master/catalog_manager.h
M src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/txn_status_entry.cc
A src/kudu/transactions/txn_status_entry.h
A src/kudu/transactions/txn_status_manager-test.cc
A src/kudu/transactions/txn_status_manager.cc
A src/kudu/transactions/txn_status_manager.h
M src/kudu/util/cow_object.h
8 files changed, 1,075 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/8
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16044
to look at the new patch set (#4).
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
KUDU-2612 p2: introduce transaction status management
This introduces the TxnStatusManager, which is backed by the
TxnStatusTablet that exposes the following APIs that will be called via
RPC, and will serve as many of the building blocks for orchestrating
two-phase commit:
- BeginTransaction: adds a new transaction under management of the
TxnStatusManager
- BeginCommitTransaction: transitions the state of a transaction from
OPEN to COMMIT_IN_PROGRESS
- AbortTransaction: transitions the state of a transaction from OPEN or
COMMIT_IN_PROGRESS to ABORTED
- RegisterParticipant: adds a participant to be associated with a
specific transaction ID
For completeness sake w.r.t defining the transaction state's enums, the
following API is also added, which will be called by the
TxnStatusManager itself upon determining a transaction has been
completed.
- FinalizeCommitTransaction: transitions the state of a transaction from
COMMIT_IN_PROGRESS to COMMITTED
This new abstraction mirrors that used by the CatalogManager, which uses
copy-on-write locking to protect concurrent access to metadata while
writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this
case, TxnStatusTablet) are being replicated.
This is at least enough of a jumping off point that we can begin
plumbing this into the tablet servers and defining an RPC service around
it -- there are still no facilities to create a TxnStatusManager.
Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
---
M src/kudu/master/catalog_manager.h
M src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/txn_status_entry.cc
A src/kudu/transactions/txn_status_entry.h
A src/kudu/transactions/txn_status_manager-test.cc
A src/kudu/transactions/txn_status_manager.cc
A src/kudu/transactions/txn_status_manager.h
M src/kudu/util/cow_object.h
8 files changed, 1,047 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/4
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 2:
(9 comments)
I did a quick first pass. Overall looks good, some nits. Will get back tomorrow after digesting it a bit :)
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_entry.h
File src/kudu/transactions/status_entry.h:
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_entry.h@105
PS2, Line 105: map
Does it make sense to use unordered_map here instead? If GetParticipantIds() is the only place where ordering is needed, I think it might make sense to sort the result container if GetOrCreateParticipant() is called much more often than GetParticipantIds().
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_entry.cc
File src/kudu/transactions/status_entry.cc:
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_entry.cc@39
PS2, Line 39: scoped_refptr<ParticipantEntry> participant;
nit: why not to have this defined and assigned at the same line?
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_entry.cc@53
PS2, Line 53: vector<string> ret;
nit: add
ret.reserve(participants_.size());
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_entry.cc@54
PS2, Line 54: for (const auto& id_and_prt : participants_) {
: ret.emplace_back(id_and_prt.first);
: }
nit: consider using AppendKeysFromMap() (it has a specialization for std::vector, so it's possible to drop ret.reserve())
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager-test.cc
File src/kudu/transactions/status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager-test.cc@383
PS2, Line 383: ASSERT_OK(txn_manager_->FinalizeCommitTransaction(kTxnId2));
Does it make sense to verify how BeginCommitTransaction() works on already committed transaction?
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager-test.cc@391
PS2, Line 391: ASSERT_OK(txn_manager_->RegisterParticipant(kTxnId1, ParticipantId(1), kOwner));
Does it make sense to add a scenario trying to register a participant when a transaction hasn't begun yet?
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager.h
File src/kudu/transactions/status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager.h@19
PS2, Line 19: stdint.h
nit: use <cstdint> instead
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager.cc
File src/kudu/transactions/status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager.cc@123
PS2, Line 123: RETURN_NOT_OK(status_tablet_.AddNewTransaction(txn_id, user));
What if there was a race and there is a record with higher transaction ID already persisted? Should TxnStatusManager handle that or that will be client handling this error case?
In either case, and for the error case above: where does the caller get an idea what ID it should try next?
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/util/cow_object.h@437
PS2, Line 437: // Helper to manage locking on the metadata protected by a CowLock.
Thank you for adding this doc!
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 23 Jun 2020 04:43:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
KUDU-2612 p2: introduce transaction status management
This introduces the TxnStatusManager, which is backed by the
TxnStatusTablet that exposes the following APIs that will be called via
RPC, and will serve as many of the building blocks for orchestrating
two-phase commit:
- BeginTransaction: adds a new transaction under management of the
TxnStatusManager
- BeginCommitTransaction: transitions the state of a transaction from
OPEN to COMMIT_IN_PROGRESS
- AbortTransaction: transitions the state of a transaction from OPEN or
COMMIT_IN_PROGRESS to ABORTED
- RegisterParticipant: adds a participant to be associated with a
specific transaction ID
For completeness sake w.r.t defining the transaction state's enums, the
following API is also added, which will be called by the
TxnStatusManager itself upon determining a transaction has been
completed.
- FinalizeCommitTransaction: transitions the state of a transaction from
COMMIT_IN_PROGRESS to COMMITTED
This new abstraction mirrors that used by the CatalogManager, which uses
copy-on-write locking to protect concurrent access to metadata while
writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this
case, TxnStatusTablet) are being replicated.
This is at least enough of a jumping off point that we can begin
plumbing this into the tablet servers and defining an RPC service around
it -- there are still no facilities to create a TxnStatusManager.
It should be noted that end-users will not call these methods directly,
but rather through some layer of indirection (e.g. clients won't request
a specific transaction ID, they'll just request to begin a transaction,
and some intermediary layer will be in charge of getting an appropriate
transaction ID). This should give us flexibility in changing the
TxnStatusManager's interface moving forward.
Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Reviewed-on: http://gerrit.cloudera.org:8080/16044
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/master/catalog_manager.h
M src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/txn_status_entry.cc
A src/kudu/transactions/txn_status_entry.h
A src/kudu/transactions/txn_status_manager-test.cc
A src/kudu/transactions/txn_status_manager.cc
A src/kudu/transactions/txn_status_manager.h
M src/kudu/util/cow_object.h
8 files changed, 1,075 insertions(+), 16 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Andrew Wong: Verified
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16044/9/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/9/src/kudu/transactions/txn_status_manager-test.cc@356
PS9, Line 356: ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString();
> Whoops, I missed this last review round: the result status is not checked.
Oops. Done
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 22:09:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 6: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@236
PS5, Line 236: string prt = ParticipantId(i % kUniqueParticipantIds);
> I wanted to exercise the error cases for registration of participants in a
Ack
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h
File src/kudu/transactions/txn_status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@77
PS5, Line 77:
> Yeah, that's a fair concern. I was hesitant to do so because I was thinking
Makes sense, thanks for the clarification.
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Jul 2020 16:57:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 6:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@281
PS6, Line 281: ReservoirSample
What is the significance of ReservoirSample() here if all_updates.size() < kNumUpdatesInParallel? As I understand, in this case not a single transaction will change its state in a concurrent manner, no?
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@328
PS6, Line 328: LOG(DFATAL) << "bad update";
Why LOG(DFATAL) is better than simply failing the test with FAIL() ?
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@348
PS6, Line 348: ASSERT_OK(txn_manager_->BeginTransaction(1, kOwner));
Do you mind adding a scenario to call BeginTransaction() after this, but call it with kWrongUser?
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@357
PS6, Line 357: ParticipantIdsByTxnId prts_by_txn_id = txn_manager_->GetParticipantsByTxnIdForTests();
What is the significance of the result? In other words, what is expected to be in the 'prts_by_txn_id' container?
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404
PS6, Line 404: can't
nit: cannot
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404
PS6, Line 404: reigster
register
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@409
PS6, Line 409: ASSERT_OK(txn_manager_->RegisterParticipant(kTxnId1, ParticipantId(1), kOwner));
Do you mind adding a scenario to show that register the same participant with the same transaction is an idempotent operation (i.e. calling this again will be a success)?
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.h
File src/kudu/transactions/txn_status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.h@114
PS6, Line 114: with that transaction ID
What's the expectation on a per-transaction set of IDs? I see the implementation sorts those, maybe it's worth reflecting that in this in-line method doc?
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h
File src/kudu/transactions/txn_status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@77
PS5, Line 77:
> Makes sense, thanks for the clarification.
We discussed this with Andrew a bit last week or so. As I understand, it's possible to change this (maybe in a follow-up changelist) so no transaction ID would be necessary even in case of sharded XST.
The idea is that the request to assign an transaction ID is sent randomly to one of XST tablets, and that tablet will read its last transaction ID record, increment it, and forward the request to create a transaction record to the appropriate partition (i.e. to other tablet of XST). That tablet on its own will check for the last transaction ID it recorded and try to insert a new record with the new ID. If failed, it will increment its last seen transaction ID and forward it to corresponding tablet XST tablet, and so on.
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc@67
PS6, Line 67: highest_txn_id_
Is it guaranteed we don't have a race here while setting the highest_txn_id_? Could you add a comment to clarify on this?
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc@94
PS6, Line 94: !ret
nit for here and elsewhere: wrap these unlikely error conditions into PREDICT_FALSE() ?
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 07 Jul 2020 18:06:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16044
to look at the new patch set (#6).
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
KUDU-2612 p2: introduce transaction status management
This introduces the TxnStatusManager, which is backed by the
TxnStatusTablet that exposes the following APIs that will be called via
RPC, and will serve as many of the building blocks for orchestrating
two-phase commit:
- BeginTransaction: adds a new transaction under management of the
TxnStatusManager
- BeginCommitTransaction: transitions the state of a transaction from
OPEN to COMMIT_IN_PROGRESS
- AbortTransaction: transitions the state of a transaction from OPEN or
COMMIT_IN_PROGRESS to ABORTED
- RegisterParticipant: adds a participant to be associated with a
specific transaction ID
For completeness sake w.r.t defining the transaction state's enums, the
following API is also added, which will be called by the
TxnStatusManager itself upon determining a transaction has been
completed.
- FinalizeCommitTransaction: transitions the state of a transaction from
COMMIT_IN_PROGRESS to COMMITTED
This new abstraction mirrors that used by the CatalogManager, which uses
copy-on-write locking to protect concurrent access to metadata while
writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this
case, TxnStatusTablet) are being replicated.
This is at least enough of a jumping off point that we can begin
plumbing this into the tablet servers and defining an RPC service around
it -- there are still no facilities to create a TxnStatusManager.
It should be noted that end-users will not call these methods directly,
but rather through some layer of indirection (e.g. clients won't request
a specific transaction ID, they'll just request to begin a transaction,
and some intermediary layer will be in charge of getting an appropriate
transaction ID). This should give us flexibility in changing the
TxnStatusManager's interface moving forward.
Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
---
M src/kudu/master/catalog_manager.h
M src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/txn_status_entry.cc
A src/kudu/transactions/txn_status_entry.h
A src/kudu/transactions/txn_status_manager-test.cc
A src/kudu/transactions/txn_status_manager.cc
A src/kudu/transactions/txn_status_manager.h
M src/kudu/util/cow_object.h
8 files changed, 1,059 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/6
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 7:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@57
PS6, Line 57: using std::string;
> warning: using decl 'shared_ptr' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@281
PS6, Line 281: vector<Status>
all_updates.size = 3 * 10, and kNumUpdatesInParallel = 20, so we're selecting a random subset to do in parallel.
> in this case not a single transaction will change its state in a concurrent manner, no?
I'm not sure I understand why this wouldn't be considered concurrent, regardless of count.
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@328
PS6, Line 328:
> Why LOG(DFATAL) is better than simply failing the test with FAIL() ?
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@348
PS6, Line 348: ASSERT_OK(txn_manager_->RegisterParticipant(1, Partic
> Do you mind adding a scenario to call BeginTransaction() after this, but ca
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@357
PS6, Line 357: s = txn_manager_->RegisterParticipant(1, ParticipantId(2), kWrongUser);
> What is the significance of the result? In other words, what is expected t
Whoops, done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404
PS6, Line 404: s.IsIlle
> register
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404
PS6, Line 404: _TRUE
> nit: cannot
This is common all over our codebase and has virtually no grammatical difference. Any particular reason?
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@409
PS6, Line 409: ST_F(TxnStatusManagerTest, TestRegisterParticipantsWithStates) {
> Do you mind adding a scenario to show that register the same participant wi
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.h
File src/kudu/transactions/txn_status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.h@114
PS6, Line 114:
> What's the expectation on a per-transaction set of IDs? I see the implemen
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc@67
PS6, Line 67: // locking.
> Is it guaranteed we don't have a race here while setting the highest_txn_id
Done
This isn't a thread-safe method. Updated the method comments and added a note here.
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc@94
PS6, Line 94: :loc
> nit for here and elsewhere: wrap these unlikely error conditions into PREDI
Done
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 07:19:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/16044/3/src/kudu/transactions/txn_status_entry.h
File src/kudu/transactions/txn_status_entry.h:
http://gerrit.cloudera.org:8080/#/c/16044/3/src/kudu/transactions/txn_status_entry.h@76
PS3, Line 76: : txn_id_(txn_id),
> warning: pass by value and use std::move [modernize-pass-by-value]
Done
http://gerrit.cloudera.org:8080/#/c/16044/3/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/3/src/kudu/transactions/txn_status_manager-test.cc@175
PS3, Line 175: // barriers.
> warning: 'emplace_back' is called inside a loop; consider pre-allocating th
Done
http://gerrit.cloudera.org:8080/#/c/16044/3/src/kudu/transactions/txn_status_manager-test.cc@178
PS3, Line 178: for (int i = 0; i < kParallelTxnsPerBatch; i++) {
> warning: 'emplace_back' is called inside a loop; consider pre-allocating th
Done
http://gerrit.cloudera.org:8080/#/c/16044/3/src/kudu/transactions/txn_status_manager-test.cc@229
PS3, Line 229: s = txn_manager_->RegisterParticipant(kTxnId, prt, kOwner);
> warning: std::move of the const variable 'prt' has no effect; remove std::m
Done
http://gerrit.cloudera.org:8080/#/c/16044/3/src/kudu/transactions/txn_status_manager-test.cc@279
PS3, Line 279: vector<thread> threads;
> warning: 'emplace_back' is called inside a loop; consider pre-allocating th
Done
http://gerrit.cloudera.org:8080/#/c/16044/3/src/kudu/transactions/txn_status_manager.h
File src/kudu/transactions/txn_status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16044/3/src/kudu/transactions/txn_status_manager.h@67
PS3, Line 67: explicit TxnStatusManager(tablet::TabletReplica* tablet_replica)
> warning: single-argument constructors must be marked explicit to avoid unin
Done
http://gerrit.cloudera.org:8080/#/c/16044/3/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16044/3/src/kudu/transactions/txn_status_manager.cc@37
PS3, Line 37: using std::vector;
> warning: using decl 'map' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 26 Jun 2020 18:01:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16044
to look at the new patch set (#3).
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
KUDU-2612 p2: introduce transaction status management
This introduces the TxnStatusManager, which is backed by the
TxnStatusTablet that exposes the following APIs that will be called via
RPC, and will serve as many of the building blocks for orchestrating
two-phase commit:
- BeginTransaction: adds a new transaction under management of the
TxnStatusManager
- BeginCommitTransaction: transitions the state of a transaction from
OPEN to COMMIT_IN_PROGRESS
- AbortTransaction: transitions the state of a transaction from OPEN or
COMMIT_IN_PROGRESS to ABORTED
- RegisterParticipant: adds a participant to be associated with a
specific transaction ID
For completeness sake w.r.t defining the transaction state's enums, the
following API is also added, which will be called by the
TxnStatusManager itself upon determining a transaction has been
completed.
- FinalizeCommitTransaction: transitions the state of a transaction from
COMMIT_IN_PROGRESS to COMMITTED
This new abstraction mirrors that used by the CatalogManager, which uses
copy-on-write locking to protect concurrent access to metadata while
writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this
case, TxnStatusTablet) are being replicated.
This is at least enough of a jumping off point that we can begin
plumbing this into the tablet servers and defining an RPC service around
it -- there are still no facilities to create a TxnStatusManager.
Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
---
M src/kudu/master/catalog_manager.h
M src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/txn_status_entry.cc
A src/kudu/transactions/txn_status_entry.h
A src/kudu/transactions/txn_status_manager-test.cc
A src/kudu/transactions/txn_status_manager.cc
A src/kudu/transactions/txn_status_manager.h
M src/kudu/util/cow_object.h
8 files changed, 1,046 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/3
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 6:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_entry.h
File src/kudu/transactions/txn_status_entry.h:
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_entry.h@43
PS5, Line 43: transa
> transaction?
Done
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@176
PS5, Line 176:
> shouldn't these be unique_ptrs?
Done
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@204
PS5, Line 204: }
> I think it helps understanding this if you add that the reason why not all
Done
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@236
PS5, Line 236: string prt = ParticipantId(i % kUniqueParticipantIds);
> why not simply registering the txn first and then registering only the part
I wanted to exercise the error cases for registration of participants in a concurrent context as well while ensuring some successes. I'll update a bit.
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@304
PS5, Line 304: threads) {
> nit: lower-case "n"
Done
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h
File src/kudu/transactions/txn_status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@55
PS5, Line 55: // Builds a TransactionEntry for the given metadata and keeps track of it in
> shouldn't these methods be commented?
Done
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@73
PS5, Line 73:
> nit: a
Done
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@77
PS5, Line 77:
> Wouldn't it be easier to change txn_id to an out parameter that is automati
Yeah, that's a fair concern. I was hesitant to do so because I was thinking that the TxnStatusManager would one day be sharded via hash partitioning too, making the returning of the next available transaction ID more complex. I was thinking that instead, the TxnManager (or a low number of TxnManagers) would keep track of the next highest transaction ID across the table, and attempt using the next available transaction ID, which might end up in a different hash of the transaction status table.
Given this table is expected to eventually be partitioned, starting out with a transaction ID at least helps us identify what tablet to send requests to, so for now it's at least serves that purpose.
I'll leave a TODO for using the next-highest transaction ID in the partition though.
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@86
PS5, Line 86:
> nit: isn't?
Done
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 30 Jun 2020 19:10:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16044
to look at the new patch set (#11).
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
KUDU-2612 p2: introduce transaction status management
This introduces the TxnStatusManager, which is backed by the
TxnStatusTablet that exposes the following APIs that will be called via
RPC, and will serve as many of the building blocks for orchestrating
two-phase commit:
- BeginTransaction: adds a new transaction under management of the
TxnStatusManager
- BeginCommitTransaction: transitions the state of a transaction from
OPEN to COMMIT_IN_PROGRESS
- AbortTransaction: transitions the state of a transaction from OPEN or
COMMIT_IN_PROGRESS to ABORTED
- RegisterParticipant: adds a participant to be associated with a
specific transaction ID
For completeness sake w.r.t defining the transaction state's enums, the
following API is also added, which will be called by the
TxnStatusManager itself upon determining a transaction has been
completed.
- FinalizeCommitTransaction: transitions the state of a transaction from
COMMIT_IN_PROGRESS to COMMITTED
This new abstraction mirrors that used by the CatalogManager, which uses
copy-on-write locking to protect concurrent access to metadata while
writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this
case, TxnStatusTablet) are being replicated.
This is at least enough of a jumping off point that we can begin
plumbing this into the tablet servers and defining an RPC service around
it -- there are still no facilities to create a TxnStatusManager.
It should be noted that end-users will not call these methods directly,
but rather through some layer of indirection (e.g. clients won't request
a specific transaction ID, they'll just request to begin a transaction,
and some intermediary layer will be in charge of getting an appropriate
transaction ID). This should give us flexibility in changing the
TxnStatusManager's interface moving forward.
Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
---
M src/kudu/master/catalog_manager.h
M src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/txn_status_entry.cc
A src/kudu/transactions/txn_status_entry.h
A src/kudu/transactions/txn_status_manager-test.cc
A src/kudu/transactions/txn_status_manager.cc
A src/kudu/transactions/txn_status_manager.h
M src/kudu/util/cow_object.h
8 files changed, 1,075 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/11
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 3:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_entry.h
File src/kudu/transactions/status_entry.h:
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_entry.h@105
PS2, Line 105:
> Does it make sense to use unordered_map here instead? If GetParticipantIds
Done
You're right, order doesn't really matter here, though it does make it easier on tests.
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_entry.cc
File src/kudu/transactions/status_entry.cc:
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_entry.cc@39
PS2, Line 39:
> nit: why not to have this defined and assigned at the same line?
Done
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_entry.cc@53
PS2, Line 53:
> nit: add
Ack
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_entry.cc@54
PS2, Line 54:
:
:
> nit: consider using AppendKeysFromMap() (it has a specialization for std::v
Done
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager-test.cc
File src/kudu/transactions/status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager-test.cc@383
PS2, Line 383:
> Does it make sense to verify how BeginCommitTransaction() works on already
Done
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager-test.cc@391
PS2, Line 391:
> Does it make sense to add a scenario trying to register a participant when
Done
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager.h
File src/kudu/transactions/status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager.h@19
PS2, Line 19:
> nit: use <cstdint> instead
Done
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager.cc
File src/kudu/transactions/status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/transactions/status_manager.cc@123
PS2, Line 123:
> What if there was a race and there is a record with higher transaction ID a
It's safe as long as no two callers of this function return success for the same txn_id. I'll add that to the header description and as a note here.
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/util/cow_object.h
File src/kudu/util/cow_object.h:
http://gerrit.cloudera.org:8080/#/c/16044/2/src/kudu/util/cow_object.h@437
PS2, Line 437: // Helper to manage locking on the metadata protected by a CowLock.
> Thank you for adding this doc!
Np, I found it difficult to follow the master's usage at first, so thought it would be helpful here.
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Jun 2020 23:25:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16044
to look at the new patch set (#9).
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
KUDU-2612 p2: introduce transaction status management
This introduces the TxnStatusManager, which is backed by the
TxnStatusTablet that exposes the following APIs that will be called via
RPC, and will serve as many of the building blocks for orchestrating
two-phase commit:
- BeginTransaction: adds a new transaction under management of the
TxnStatusManager
- BeginCommitTransaction: transitions the state of a transaction from
OPEN to COMMIT_IN_PROGRESS
- AbortTransaction: transitions the state of a transaction from OPEN or
COMMIT_IN_PROGRESS to ABORTED
- RegisterParticipant: adds a participant to be associated with a
specific transaction ID
For completeness sake w.r.t defining the transaction state's enums, the
following API is also added, which will be called by the
TxnStatusManager itself upon determining a transaction has been
completed.
- FinalizeCommitTransaction: transitions the state of a transaction from
COMMIT_IN_PROGRESS to COMMITTED
This new abstraction mirrors that used by the CatalogManager, which uses
copy-on-write locking to protect concurrent access to metadata while
writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this
case, TxnStatusTablet) are being replicated.
This is at least enough of a jumping off point that we can begin
plumbing this into the tablet servers and defining an RPC service around
it -- there are still no facilities to create a TxnStatusManager.
It should be noted that end-users will not call these methods directly,
but rather through some layer of indirection (e.g. clients won't request
a specific transaction ID, they'll just request to begin a transaction,
and some intermediary layer will be in charge of getting an appropriate
transaction ID). This should give us flexibility in changing the
TxnStatusManager's interface moving forward.
Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
---
M src/kudu/master/catalog_manager.h
M src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/txn_status_entry.cc
A src/kudu/transactions/txn_status_entry.h
A src/kudu/transactions/txn_status_manager-test.cc
A src/kudu/transactions/txn_status_manager.cc
A src/kudu/transactions/txn_status_manager.h
M src/kudu/util/cow_object.h
8 files changed, 1,075 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/9
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 22:24:35 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@281
PS6, Line 281: ReservoirSample
> all_updates.size = 3 * 10, and kNumUpdatesInParallel = 20, so we're selecti
Ah, it seems that was just a misunderstanding from my side: there are three elements per transaction ID, so all is well. Sorry for the noise.
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@328
PS6, Line 328: LOG(DFATAL) << "bad update";
> Done
I still see LOG(DFATAL) instead of FAIL() here. It seems the usage of these macros should be swapped for lines 327 vs 299 ?
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404
PS6, Line 404: can't
> This is common all over our codebase and has virtually no grammatical diffe
IIRC, in writing it's more common to avoid the contraction and use "cannot" vs "can't", while in non-formal speech "can't" is used. Not sure where to put these in-line comments in the code, but I thought it would be more in the "writing" domain.
Feel free to ignore this, it's just a nit.
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h
File src/kudu/transactions/txn_status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@77
PS5, Line 77:
> Thanks for summarizing the discussion Alexey! I amended the TODO to be more
Thank you for updates Andrew!
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 21:14:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16044
to look at the new patch set (#10).
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
KUDU-2612 p2: introduce transaction status management
This introduces the TxnStatusManager, which is backed by the
TxnStatusTablet that exposes the following APIs that will be called via
RPC, and will serve as many of the building blocks for orchestrating
two-phase commit:
- BeginTransaction: adds a new transaction under management of the
TxnStatusManager
- BeginCommitTransaction: transitions the state of a transaction from
OPEN to COMMIT_IN_PROGRESS
- AbortTransaction: transitions the state of a transaction from OPEN or
COMMIT_IN_PROGRESS to ABORTED
- RegisterParticipant: adds a participant to be associated with a
specific transaction ID
For completeness sake w.r.t defining the transaction state's enums, the
following API is also added, which will be called by the
TxnStatusManager itself upon determining a transaction has been
completed.
- FinalizeCommitTransaction: transitions the state of a transaction from
COMMIT_IN_PROGRESS to COMMITTED
This new abstraction mirrors that used by the CatalogManager, which uses
copy-on-write locking to protect concurrent access to metadata while
writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this
case, TxnStatusTablet) are being replicated.
This is at least enough of a jumping off point that we can begin
plumbing this into the tablet servers and defining an RPC service around
it -- there are still no facilities to create a TxnStatusManager.
It should be noted that end-users will not call these methods directly,
but rather through some layer of indirection (e.g. clients won't request
a specific transaction ID, they'll just request to begin a transaction,
and some intermediary layer will be in charge of getting an appropriate
transaction ID). This should give us flexibility in changing the
TxnStatusManager's interface moving forward.
Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
---
M src/kudu/master/catalog_manager.h
M src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/txn_status_entry.cc
A src/kudu/transactions/txn_status_entry.h
A src/kudu/transactions/txn_status_manager-test.cc
A src/kudu/transactions/txn_status_manager.cc
A src/kudu/transactions/txn_status_manager.h
M src/kudu/util/cow_object.h
8 files changed, 1,075 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/10
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 22:59:42 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
......................................................................
Patch Set 11: Verified+1
timeout of RefreshTableLocationsTest.ThunderingHerd seems unrelated
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 09 Jul 2020 21:34:53 +0000
Gerrit-HasComments: No