You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/10/12 21:59:23 UTC
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16586
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
KUDU-2612: add TxnManager::BeginTransaction()
This patch adds the implementation of the BeginTransaction() method
to the TxnManager along with tests to cover the newly introduced
functionality.
Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
---
M src/kudu/master/txn_manager-test.cc
M src/kudu/master/txn_manager.cc
M src/kudu/tserver/tablet_service.cc
3 files changed, 242 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/16586/1
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16586/3/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/3/src/kudu/master/txn_manager.cc@184
PS3, Line 184: try_txn_id / txn_status_table_range_span_) *
: txn_status_table_range_span_;
: const auto hb = lb + txn_status_table_range_span_;
> If FLAGS_txn_manager_status_table_range_partition_span is changed, it's ne
I see. Yep, makes sense we'd have to examine the highest existing range.
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 14 Oct 2020 18:52:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
Patch Set 3:
(1 comment)
Thank you for review!
http://gerrit.cloudera.org:8080/#/c/16586/3/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/3/src/kudu/master/txn_manager.cc@184
PS3, Line 184: try_txn_id / txn_status_table_range_span_) *
: txn_status_table_range_span_;
: const auto hb = lb + txn_status_table_range_span_;
> I see. Yep, makes sense we'd have to examine the highest existing range.
I'm planning to address the TODO above in a follow-up patch. I think it's not a high priority item, but of course it introduced a bit of technical debt. I'm planning to get back to related items once the client-side API for transaction functionality is taken care of.
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 14 Oct 2020 19:33:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16586/3/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/3/src/kudu/master/txn_manager.cc@184
PS3, Line 184: try_txn_id / txn_status_table_range_span_) *
: txn_status_table_range_span_;
: const auto hb = lb + txn_status_table_range_span_;
> I'm not sure I see how this works if txn_status_table_range_span_ is differ
If FLAGS_txn_manager_status_table_range_partition_span is changed, it's necessary to drop existing range partitions in the transaction status table. Otherwise, it will brake here unless the TODO above is addressed.
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 14 Oct 2020 18:42:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc
File src/kudu/master/txn_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc@371
PS1, Line 371: proxy_
Should be 'p'?
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc@374
PS1, Line 374:
nit: spacing?
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc@399
PS1, Line 399: txn_initiator(kNumTransactions, nullptr);
Would it make sense to also check that we have kNumTransactions transaction IDs here?
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc@426
PS1, Line 426: insert
nit: emplace?
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager.cc@119
PS1, Line 119: while (true) {
: if (next_txn_id_.compare_exchange_strong(stored_txn_id, try_txn_id + 1) ||
: stored_txn_id > try_txn_id) {
: break;
: }
: }
I don't fully understand this. Wouldn't try_txn_id + 1 = next_txn_id_ in the common case? If that's the case, isn't this exchange a no-op? Would you mind adding a comment explaining this block? I think we should be trying to set next_txn_id_ to max(next_txn_id_, stored_txn_id), right?
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager.cc@134
PS1, Line 134: if (s.IsNotFound()) {
Maybe not as a part of this patch, but I'm curious whether you think it's worth adding a CheckInitialized()-style return here in case we're already trying to add a range?
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 13 Oct 2020 00:10:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc@124
PS2, Line 124: next_txn_id_.compare_exchange_strong(stored_txn_id,
: highest_seen_txn_id + 1) ||
: stored_txn_id > highest_seen_txn_id) {
> Maybe reverse the order so we don't have to compare the atomic if stored_tx
I don't think reversing the order make sense: stored_txn_id is updated by the compare_exchange_strong() if it returns false, so it's essential to get its value _after_ the call to compare_exchange_strong.
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 14 Oct 2020 00:02:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager-test.cc
File src/kudu/master/txn_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager-test.cc@405
PS2, Line 405: static constexpr int64_t kNumTransactions = 10000;
> nit: Would it also make sense to check that we have added several new table
Good point! Done.
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager-test.cc@407
PS2, Line 407: txn_initiator(kNumTransactions, &txn_ids);
> nit: not that we expect concurrency issues here, but maybe also dedupe thes
Good point! Done. I added a check for the monotonicity of txn_ids.
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc@123
PS2, Line 123: while (true) {
> nit: should we DCHECK that highest_seen_txn_id >= 0 here too?
Done
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 14 Oct 2020 06:08:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
Patch Set 2: Code-Review+1
(6 comments)
Overall LGTM. Just some nits left.
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc
File src/kudu/master/txn_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc@426
PS1, Line 426:
> I'm not sure emplace() would fit here: that's a range insert. At least, I
Yep, you're right. emplace() doesn't take in iterators like this.
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager-test.cc
File src/kudu/master/txn_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager-test.cc@405
PS2, Line 405: static constexpr int64_t kNumTransactions = 10000;
nit: Would it also make sense to check that we have added several new tablets for the transaction status table, to more explicitly test the AddNewRangePartition logic?
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager-test.cc@407
PS2, Line 407: txn_initiator(kNumTransactions, &txn_ids);
nit: not that we expect concurrency issues here, but maybe also dedupe these before comparing?
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager.cc@119
PS1, Line 119: // with the highest 'highest_seen_txn_id' so far updating 'next_txn_id_'
: // until it succeeds or bail if there is another thread that received a
: // greater 'highest_seen_txn_id' back from TxnStatusManager.
: int64_t stored_txn_id = try_txn_id + 1;
: while (true) {
:
> Ah, I guess there was a swap of the 'try_txn_id' and 'highest_seen_txn_id',
Yep, this is a lot clearer now.
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc@123
PS2, Line 123: while (true) {
nit: should we DCHECK that highest_seen_txn_id >= 0 here too?
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc@124
PS2, Line 124: next_txn_id_.compare_exchange_strong(stored_txn_id,
: highest_seen_txn_id + 1) ||
: stored_txn_id > highest_seen_txn_id) {
Maybe reverse the order so we don't have to compare the atomic if stored_txn_id > highest_seen_txn_id?
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 13 Oct 2020 23:02:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16586
to look at the new patch set (#2).
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
KUDU-2612: add TxnManager::BeginTransaction()
This patch adds the implementation of the BeginTransaction() method
to the TxnManager along with tests to cover the newly introduced
functionality.
Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
---
M src/kudu/master/txn_manager-test.cc
M src/kudu/master/txn_manager.cc
M src/kudu/transactions/txn_system_client.cc
M src/kudu/tserver/tablet_service.cc
4 files changed, 247 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/16586/2
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
KUDU-2612: add TxnManager::BeginTransaction()
This patch adds the implementation of the BeginTransaction() method
to the TxnManager along with tests to cover the newly introduced
functionality.
Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Reviewed-on: http://gerrit.cloudera.org:8080/16586
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/master/txn_manager-test.cc
M src/kudu/master/txn_manager.cc
M src/kudu/master/txn_manager.h
M src/kudu/transactions/txn_system_client.cc
M src/kudu/tserver/tablet_service.cc
5 files changed, 301 insertions(+), 23 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc
File src/kudu/master/txn_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc@371
PS1, Line 371: proxy_
> Should be 'p'?
Indeed. Thanks!
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc@374
PS1, Line 374:
> nit: spacing?
Done
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc@399
PS1, Line 399: txn_initiator(kNumTransactions, nullptr);
> Would it make sense to also check that we have kNumTransactions transaction
Done
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc@426
PS1, Line 426: insert
> nit: emplace?
I'm not sure emplace() would fit here: that's a range insert. At least, I don't see range std::unordered_set::emplace in the C++ stdlib API: https://en.cppreference.com/w/cpp/container/unordered_set
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager.cc@119
PS1, Line 119: while (true) {
: if (next_txn_id_.compare_exchange_strong(stored_txn_id, try_txn_id + 1) ||
: stored_txn_id > try_txn_id) {
: break;
: }
: }
> I don't fully understand this. Wouldn't try_txn_id + 1 = next_txn_id_ in th
Ah, I guess there was a swap of the 'try_txn_id' and 'highest_seen_txn_id', indeed. Most likely I messed it up while doing second iteration or so. Good catch!
The idea is to make the thread that has gotten a transaction reserved with the highest 'highest_seen_txn_id' so far updating 'next_txn_id_' until it succeeds or bail if there is another thread that received a greater 'highest_seen_txn_id' back from TxnStatusManager.
With this approach, max(next_txn_id_, stored_txn_id_) isn't needed since it's done automatically.
Is it clearer now with try_txn_id <--> highest_seen_txn_id swap and the comment added?
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager.cc@134
PS1, Line 134: if (s.IsNotFound()) {
> Maybe not as a part of this patch, but I'm curious whether you think it's w
Yep, good point. I think we can track whether a request to add some particular range has already been sent. I guess that would require tracking the range since we cannot assume that's a single range all over the place because the rate of incoming BeginTransaction request might be high. So, I'd rather put it into a separate patch to 1) add targeted tests as well 2) make reviewing easier.
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 13 Oct 2020 01:21:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16586/3/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/3/src/kudu/master/txn_manager.cc@184
PS3, Line 184: try_txn_id / txn_status_table_range_span_) *
: txn_status_table_range_span_;
: const auto hb = lb + txn_status_table_range_span_;
I'm not sure I see how this works if txn_status_table_range_span_ is different from FLAGS_txn_manager_status_table_range_partition_span. It's not listed as runtime, but isn't it possible that it's changed?
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 14 Oct 2020 17:58:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16586
to look at the new patch set (#3).
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
KUDU-2612: add TxnManager::BeginTransaction()
This patch adds the implementation of the BeginTransaction() method
to the TxnManager along with tests to cover the newly introduced
functionality.
Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
---
M src/kudu/master/txn_manager-test.cc
M src/kudu/master/txn_manager.cc
M src/kudu/master/txn_manager.h
M src/kudu/transactions/txn_system_client.cc
M src/kudu/tserver/tablet_service.cc
5 files changed, 301 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/16586/3
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc@124
PS2, Line 124: next_txn_id_.compare_exchange_strong(stored_txn_id,
: highest_seen_txn_id + 1) ||
: stored_txn_id > highest_seen_txn_id) {
> I don't think reversing the order make sense: stored_txn_id is updated by t
Ah right. SGTM.
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 14 Oct 2020 00:11:04 +0000
Gerrit-HasComments: Yes