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/12/03 19:53:59 UTC
[kudu-CR] [TxnStatusManagerTest] fix replica restart in GetTransactionStatus
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16818
Change subject: [TxnStatusManagerTest] fix replica restart in GetTransactionStatus
......................................................................
[TxnStatusManagerTest] fix replica restart in GetTransactionStatus
Before this patch, there was an issue with accessing already deleted
txn_manager_ member in the TxnStatusManagerTest.TxnStatusManagerTest
scenario and ASAN rightfully reported on that:
src/kudu/transactions/txn_status_manager.cc:161:53: runtime error: \
member call on address 0x6150000e7480 which does not point to an \
object of type 'kudu::tablet::TabletReplica'
0x6150000e7480: note: object has invalid vptr
Change-Id: I62f86cfc686a121a1689e6f3942aa4050406ffe2
---
M src/kudu/transactions/txn_status_manager-test.cc
1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/16818/1
--
To view, visit http://gerrit.cloudera.org:8080/16818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I62f86cfc686a121a1689e6f3942aa4050406ffe2
Gerrit-Change-Number: 16818
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [TxnStatusManagerTest] fix replica restart in GetTransactionStatus
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [TxnStatusManagerTest] fix replica restart in GetTransactionStatus
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I62f86cfc686a121a1689e6f3942aa4050406ffe2
Gerrit-Change-Number: 16818
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)
[kudu-CR] [TxnStatusManagerTest] fix replica restart in GetTransactionStatus
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16818 )
Change subject: [TxnStatusManagerTest] fix replica restart in GetTransactionStatus
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16818/1/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16818/1/src/kudu/transactions/txn_status_manager-test.cc@474
PS1, Line 474: {
: ASSERT_OK(RestartReplica());
: decltype(txn_manager_) txn_manager_reloaded(
: new TxnStatusManager(tablet_replica_.get()));
: ASSERT_OK(txn_manager_reloaded->LoadFromTablet());
: txn_manager_ = std::move(txn_manager_reloaded);
: }
> At first, I thought it might be worth encapsulating this in some RestartRep
Right -- I was also thinking about that, and making RestartReplica() to be a virtual method. Since it's more a unit test, I guess current approach is good enough for a while. We can change it in future if we want.
--
To view, visit http://gerrit.cloudera.org:8080/16818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62f86cfc686a121a1689e6f3942aa4050406ffe2
Gerrit-Change-Number: 16818
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: Thu, 03 Dec 2020 20:37:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TxnStatusManagerTest] fix replica restart in GetTransactionStatus
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16818 )
Change subject: [TxnStatusManagerTest] fix replica restart in GetTransactionStatus
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Thanks for finding and fixing this!
http://gerrit.cloudera.org:8080/#/c/16818/1/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16818/1/src/kudu/transactions/txn_status_manager-test.cc@474
PS1, Line 474: {
: ASSERT_OK(RestartReplica());
: decltype(txn_manager_) txn_manager_reloaded(
: new TxnStatusManager(tablet_replica_.get()));
: ASSERT_OK(txn_manager_reloaded->LoadFromTablet());
: txn_manager_ = std::move(txn_manager_reloaded);
: }
At first, I thought it might be worth encapsulating this in some RestartReplicaAndReload(), but it seems that elsewhere, RestartReplica() calls are followed by the creation of separate status manager instances rather than using txn_manager_, so it probably wouldn't be useful with the current set of tests beyond this one.
--
To view, visit http://gerrit.cloudera.org:8080/16818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62f86cfc686a121a1689e6f3942aa4050406ffe2
Gerrit-Change-Number: 16818
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: Thu, 03 Dec 2020 20:25:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TxnStatusManagerTest] fix replica restart in GetTransactionStatus
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16818 )
Change subject: [TxnStatusManagerTest] fix replica restart in GetTransactionStatus
......................................................................
[TxnStatusManagerTest] fix replica restart in GetTransactionStatus
Before this patch, there was an issue with accessing already deleted
txn_manager_ member in the TxnStatusManagerTest.TxnStatusManagerTest
scenario and ASAN rightfully reported on that:
src/kudu/transactions/txn_status_manager.cc:161:53: runtime error: \
member call on address 0x6150000e7480 which does not point to an \
object of type 'kudu::tablet::TabletReplica'
0x6150000e7480: note: object has invalid vptr
Change-Id: I62f86cfc686a121a1689e6f3942aa4050406ffe2
Reviewed-on: http://gerrit.cloudera.org:8080/16818
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/transactions/txn_status_manager-test.cc
1 file changed, 7 insertions(+), 1 deletion(-)
Approvals:
Andrew Wong: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/16818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I62f86cfc686a121a1689e6f3942aa4050406ffe2
Gerrit-Change-Number: 16818
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)