[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16526 ) Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB .. Patch Set 6: Code-Review+2 Carrying over Hao Hao's +2 from PS5. -- To view, visit http://gerrit.cloudera.org:8080/16526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 Gerrit-Change-Number: 16526 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 08 Oct 2020 03:21:09 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16526 ) Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB .. KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB This patch adds the highest transaction identifier seen by the TxnStatusManager (TxnStatusManager coordinates the lifecycle of transactional operations for a particular tablet of the transaction status table). The new field is populated when responding to CoordinateTransaction() RPC of the BEGIN_TXN type. The signature and the implementation of the related methods in the TxnStatusManager and TxnSystemClient classes have been updated as well, along with corresponding tests to cover the newly introduced functionality. The newly introduced field will be used in a follow-up patch containing the initial implementation of the TxnManager component. The rationale behind having this new field is to allow the TxnManager to 'calibrate' its last seen txn_id, so it could make less trial-and-error iterations when reserving an identifier for a transaction. The CoordinatorOpResultPB::highest_seen_txn_id would not be needed if TxnStatusManager (or their federation) was able to reserve an identifier for a new transaction on their own. Of course, that would require some sort of interaction among TxnStatusManager instances (like request forwarding or alike). But without such a provision, the TxnManager needs the newly introduced field to succeed with its trial-and-error approach while finding the next available identifier for a newly initiated transaction. This is a follow-up to 95f4109518e7b81b9115a4e8bacbd157dcecad0c. Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 Reviewed-on: http://gerrit.cloudera.org:8080/16526 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/integration-tests/txn_status_table-itest.cc M src/kudu/tablet/txn_coordinator.h 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 M src/kudu/transactions/txn_system_client.cc M src/kudu/transactions/txn_system_client.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver_admin.proto 10 files changed, 177 insertions(+), 48 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/16526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 Gerrit-Change-Number: 16526 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/16526 )
Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into
CoordinatorOpResultPB
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/transactions/txn_status_manager.cc@164
PS3, Line 164: // Always set the 'highest_seen_txn_id' on return.
: SCOPED_CLEANUP({
: if (highest_seen_txn_id) {
: std::lock_guard l(lock_);
: *highest_seen_txn_id = highest_txn_id_;
: }
: });
> Sounds good to me, would you mind adding more to the comment to explain the
@awong: the issues with extra lock acquisition and posting back -2 and -1 magic
number are addressed now
@hao.hao: I added a blurb to explain to motivation behind all this fuss
--
To view, visit http://gerrit.cloudera.org:8080/16526
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0
Gerrit-Change-Number: 16526
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 08 Oct 2020 00:16:22 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16526 to look at the new patch set (#6). Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB .. KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB This patch adds the highest transaction identifier seen by the TxnStatusManager (TxnStatusManager coordinates the lifecycle of transactional operations for a particular tablet of the transaction status table). The new field is populated when responding to CoordinateTransaction() RPC of the BEGIN_TXN type. The signature and the implementation of the related methods in the TxnStatusManager and TxnSystemClient classes have been updated as well, along with corresponding tests to cover the newly introduced functionality. The newly introduced field will be used in a follow-up patch containing the initial implementation of the TxnManager component. The rationale behind having this new field is to allow the TxnManager to 'calibrate' its last seen txn_id, so it could make less trial-and-error iterations when reserving an identifier for a transaction. The CoordinatorOpResultPB::highest_seen_txn_id would not be needed if TxnStatusManager (or their federation) was able to reserve an identifier for a new transaction on their own. Of course, that would require some sort of interaction among TxnStatusManager instances (like request forwarding or alike). But without such a provision, the TxnManager needs the newly introduced field to succeed with its trial-and-error approach while finding the next available identifier for a newly initiated transaction. This is a follow-up to 95f4109518e7b81b9115a4e8bacbd157dcecad0c. Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 --- M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/integration-tests/txn_status_table-itest.cc M src/kudu/tablet/txn_coordinator.h 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 M src/kudu/transactions/txn_system_client.cc M src/kudu/transactions/txn_system_client.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver_admin.proto 10 files changed, 177 insertions(+), 48 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/16526/6 -- To view, visit http://gerrit.cloudera.org:8080/16526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 Gerrit-Change-Number: 16526 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/16526 )
Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into
CoordinatorOpResultPB
..
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/transactions/txn_status_manager.cc@164
PS3, Line 164: // Always set the 'highest_seen_txn_id' on return.
: SCOPED_CLEANUP({
: if (highest_seen_txn_id) {
: std::lock_guard l(lock_);
: *highest_seen_txn_id = highest_txn_id_;
: }
: });
> OK sounds reasonable, particularly with concurrent activity. My main concer
Sounds good to me, would you mind adding more to the comment to explain the
motivation? Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/16526
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0
Gerrit-Change-Number: 16526
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 07 Oct 2020 23:02:03 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16526 )
Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into
CoordinatorOpResultPB
..
Patch Set 4: Code-Review+1
(1 comment)
Will leave this open since I think Hao should also review the pieces that are
relevant to her tablet-loading work.
http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/transactions/txn_status_manager.cc@164
PS3, Line 164: // Always set the 'highest_seen_txn_id' on return.
: SCOPED_CLEANUP({
: if (highest_seen_txn_id) {
: std::lock_guard l(lock_);
: *highest_seen_txn_id = highest_txn_id_;
: }
: });
> I was considering that as the very first approach, but I'd like to have 'hi
OK sounds reasonable, particularly with concurrent activity. My main concern is
that exposing the transaction IDs on all errors might open the door for relying
on the -2 and -1 magic numbers instead of the 'ts_error' or return status. I'd
rather not have anything rely on such magic numbers because I expect the
approach to change near-term since Hao is working on loading the tablet upon
leadership change, in which case the "loaded" state might be tracked separately.
That said, that sort of thing should be easily caught via code review, and also
if we treat the 'highest_seen_txn_id' as undefined for all non-InvalidArgument
errors.
--
To view, visit http://gerrit.cloudera.org:8080/16526
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0
Gerrit-Change-Number: 16526
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 03 Oct 2020 01:39:59 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16526 ) Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/tablet/txn_coordinator.h File src/kudu/tablet/txn_coordinator.h: http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/tablet/txn_coordinator.h@58 PS3, Line 58: // TODO(aserbin): document the 'highest_seen_txn_id' parameter > Can this be done in this patch, or is there additional functionality you're Done -- To view, visit http://gerrit.cloudera.org:8080/16526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 Gerrit-Change-Number: 16526 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 02 Oct 2020 21:22:37 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16526 to look at the new patch set (#4). Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB .. KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB This patch adds the highest transaction identifier seen by the TxnStatusManager (TxnStatusManager coordinates the lifecycle of transactional operations for a particular tablet of the transaction status table). The new field is populated when responding to CoordinateTransaction() RPC of the BEGIN_TXN type. The signature and the implementation of the related methods in the TxnStatusManager and TxnSystemClient classes have been updated as well, along with corresponding tests to cover the newly introduced functionality. The newly introduced field will be used in a follow-up patch containing the initial implementation of the TxnManager component. The rationale behind having this new field is to allow the TxnManager to 'calibrate' its last seen txn_id, so it could make less trial-and-error iterations when reserving an identifier for a transaction. The CoordinatorOpResultPB::highest_seen_txn_id would not be needed if TxnStatusManager (or their federation) was able to reserve an identifier for a new transaction on their own. Of course, that would require some sort of interaction among TxnStatusManager instances (like request forwarding or alike). But without such a provision, the TxnManager needs the newly introduced field to succeed with its trial-and-error approach while finding the next available identifier for a newly initiated transaction. This is a follow-up to 95f4109518e7b81b9115a4e8bacbd157dcecad0c. Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 --- M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/integration-tests/txn_status_table-itest.cc M src/kudu/tablet/txn_coordinator.h 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 M src/kudu/transactions/txn_system_client.cc M src/kudu/transactions/txn_system_client.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver_admin.proto 10 files changed, 155 insertions(+), 48 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/16526/4 -- To view, visit http://gerrit.cloudera.org:8080/16526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 Gerrit-Change-Number: 16526 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/16526 )
Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into
CoordinatorOpResultPB
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/transactions/txn_status_manager.cc@164
PS3, Line 164: // Always set the 'highest_seen_txn_id' on return.
: SCOPED_CLEANUP({
: if (highest_seen_txn_id) {
: std::lock_guard l(lock_);
: *highest_seen_txn_id = highest_txn_id_;
: }
: });
> Actually, rather than dropping and re-taking locks via this SCOPED_CLEANUP,
I was considering that as the very first approach, but I'd like to have
'highest_seen_txn_id' even when returning from this method with an error from
status_tablet_.AddNewTransaction(txn_id, user, ts_error). In case of
concurrent activity, highest_txn_id_ might change.
--
To view, visit http://gerrit.cloudera.org:8080/16526
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0
Gerrit-Change-Number: 16526
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Oct 2020 18:49:36 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16526 )
Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into
CoordinatorOpResultPB
..
Patch Set 3: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/tablet/txn_coordinator.h
File src/kudu/tablet/txn_coordinator.h:
http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/tablet/txn_coordinator.h@58
PS3, Line 58: // TODO(aserbin): document the 'highest_seen_txn_id' parameter
Can this be done in this patch, or is there additional functionality you're
waiting on?
http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16526/3/src/kudu/transactions/txn_status_manager.cc@164
PS3, Line 164: // Always set the 'highest_seen_txn_id' on return.
: SCOPED_CLEANUP({
: if (highest_seen_txn_id) {
: std::lock_guard l(lock_);
: *highest_seen_txn_id = highest_txn_id_;
: }
: });
Actually, rather than dropping and re-taking locks via this SCOPED_CLEANUP, how
would you feel about just explicitly putting these in the below lock_ block. We
then would also not set this if the tablet isn't loaded.
--
To view, visit http://gerrit.cloudera.org:8080/16526
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0
Gerrit-Change-Number: 16526
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Oct 2020 18:05:38 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16526 to look at the new patch set (#3). Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB .. KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB This patch adds the highest transaction identifier seen by the TxnStatusManager (TxnStatusManager coordinates the lifecycle of transactional operations for a particular tablet of the transaction status table). The new field is populated when responding to CoordinateTransaction() RPC of the BEGIN_TXN type. The signature and the implementation of the related methods in the TxnStatusManager and TxnSystemClient classes have been updated as well, along with corresponding tests to cover the newly introduced functionality. The newly introduced field will be used in a follow-up patch containing the initial implementation of the TxnManager component. The rationale behind having this new field is to allow the TxnManager to 'calibrate' its last seen txn_id, so it could make less trial-and-error iterations when reserving an identifier for a transaction. The CoordinatorOpResultPB::highest_seen_txn_id would not be needed if TxnStatusManager (or their federation) was able to reserve an identifier for a new transaction on their own. Of course, that would require some sort of interaction among TxnStatusManager instances (like request forwarding or alike). But without such a provision, the TxnManager needs the newly introduced field to succeed with its trial-and-error approach while finding the next available identifier for a newly initiated transaction. This is a follow-up to 95f4109518e7b81b9115a4e8bacbd157dcecad0c. Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 --- M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/integration-tests/txn_status_table-itest.cc M src/kudu/tablet/txn_coordinator.h 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 M src/kudu/transactions/txn_system_client.cc M src/kudu/transactions/txn_system_client.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver_admin.proto 10 files changed, 155 insertions(+), 47 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/16526/3 -- To view, visit http://gerrit.cloudera.org:8080/16526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 Gerrit-Change-Number: 16526 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/16526 )
Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into
CoordinatorOpResultPB
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16526/1/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16526/1/src/kudu/transactions/txn_status_manager.cc@121
PS1, Line 121: std::lock_guard l(lock_);
: if (highest_seen_txn_id) {
: *highest_seen_txn_id = highest_txn_id_;
: }
> Could we only take the lock if 'highest_seen_txn_id' is set?
Yep, taking a lock only when highest_seen_txn_id != nullptr is a good idea.
Done.
The idea is to populate the 'highest_seen_txn_id' parameter in case of success,
InvalidArgument, and other cases to keep to TxnManger updated and make the
logic there simpler. We always can report back highest_txn_id from there
because we know it. Populating the 'highest_seen_txn_id' output parameter in
the success case is also important in case of higher contention: e.g., there
might be another call to this method that increments highest_txn_id_ before
control reaches the end of this method for current thread.
I think we could switch to an atomic for 'highest_txn_id_' to avoid holding a
lock in the scoped cleanup if we find that it's a bottleneck. I guess we first
need to add a small benchmark test for that to make an informed decision.
--
To view, visit http://gerrit.cloudera.org:8080/16526
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0
Gerrit-Change-Number: 16526
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 02 Oct 2020 08:12:21 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Hello Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16526 to look at the new patch set (#2). Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB .. KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB This patch adds the highest transaction identifier seen by the TxnStatusManager (TxnStatusManager coordinates the lifecycle of transactional operations for a particular tablet of the transaction status table). The new field is populated when responding to CoordinateTransaction() RPC of the BEGIN_TXN type. The signature and the implementation of the related methods in the TxnStatusManager and TxnSystemClient classes have been updated as well, along with corresponding tests to cover the newly introduced functionality. The newly introduced field will be used in a follow-up patch containing the initial implementation of the TxnManager component. The rationale behind having this new field is to allow the TxnManager to 'calibrate' its last seen txn_id, so it could make less trial-and-error iterations when reserving an identifier for a transaction. The CoordinatorOpResultPB::highest_seen_txn_id would not be needed if TxnStatusManager (or their federation) was able to reserve an identifier for a new transaction on their own. Of course, that would require some sort of interaction among TxnStatusManager instances (like request forwarding or alike). But without such a provision, the TxnManager needs the newly introduced field to succeed with its trial-and-error approach while finding the next available identifier for a newly initiated transaction. This is a follow-up to 95f4109518e7b81b9115a4e8bacbd157dcecad0c. Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 --- M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/integration-tests/txn_status_table-itest.cc M src/kudu/tablet/txn_coordinator.h 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 M src/kudu/transactions/txn_system_client.cc M src/kudu/transactions/txn_system_client.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver_admin.proto 10 files changed, 154 insertions(+), 46 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/16526/2 -- To view, visit http://gerrit.cloudera.org:8080/16526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 Gerrit-Change-Number: 16526 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16526 )
Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into
CoordinatorOpResultPB
..
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16526/1/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16526/1/src/kudu/transactions/txn_status_manager.cc@121
PS1, Line 121: std::lock_guard l(lock_);
: if (highest_seen_txn_id) {
: *highest_seen_txn_id = highest_txn_id_;
: }
Could we only take the lock if 'highest_seen_txn_id' is set?
Alternatively, should we only set this on InvalidArgument or success? To avoid
the extra taking of the lock?
--
To view, visit http://gerrit.cloudera.org:8080/16526
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0
Gerrit-Change-Number: 16526
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 01 Oct 2020 22:00:19 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p5 (b): add highest seen txn id into CoordinatorOpResultPB
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16526 Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB .. KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB This patch adds the highest transaction identifier seen by the TxnStatusManager (TxnStatusManager coordinates the lifecycle of transactional operations for a particular tablet of the transaction status table). The new field is populated when responding to CoordinateTransaction() RPC of the BEGIN_TXN type. The signature and the implementation of the related methods in the TxnStatusManager and TxnSystemClient classes have been updated as well, along with corresponding tests to cover the newly introduced functionality. The newly introduced field will be used in a follow-up patch containing the initial implementation of the TxnManager component. The rationale behind having this new field is to allow the TxnManager to 'calibrate' its last seen txn_id, so it could make less trial-and-error iterations when reserving an identifier for a transaction. The CoordinatorOpResultPB::highest_seen_txn_id would not be needed if TxnStatusManager (or their federation) was able to reserve an identifier for a new transaction on their own. Of course, that would require some sort of interaction among TxnStatusManager instances (like request forwarding or alike). But without such a provision, the TxnManager needs the newly introduced field to succeed with its trial-and-error approach while finding the next available identifier for a newly initiated transaction. This is a follow-up to 95f4109518e7b81b9115a4e8bacbd157dcecad0c. Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 --- M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/integration-tests/txn_status_table-itest.cc M src/kudu/tablet/txn_coordinator.h 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 M src/kudu/transactions/txn_system_client.cc M src/kudu/transactions/txn_system_client.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver_admin.proto 10 files changed, 154 insertions(+), 46 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/16526/1 -- To view, visit http://gerrit.cloudera.org:8080/16526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 Gerrit-Change-Number: 16526 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
