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 2021/06/21 20:15:32 UTC
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17611
Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync()
......................................................................
[txn_system_client] MonoDelta by value in CoordinateTransactionAsync()
This patch updates the signature of the
TxnSystemClient::CoordinateTransactionAsync() method to pass MonoDelta
by value. In addition, I added an extra DCHECK() into
MonoTime::AddDelta().
The motivation for this change was seeing the following UBSAN warning
in one of the pre-commit builds [1]:
#0 0x7f48d11e8f3c in kudu::MonoTime::AddDelta(kudu::MonoDelta const&) src/kudu/util/monotime.cc:218:10
#1 0x7f48d11e9eee in kudu::operator+(kudu::MonoTime const&, kudu::MonoDelta const&) src/kudu/util/monotime.cc:333:7
#2 0x7f48e0d846c1 in kudu::transactions::TxnSystemClient::CoordinateTransactionAsync(kudu::tserver::CoordinatorOpPB, kudu::MonoDelta const&, std::function<void (kudu::Status const&)> const&, kudu::tserver::CoordinatorOpResultPB*) src/kudu/transactions/txn_system_client.cc:331:45
#3 0x7f48e0d86f76 in kudu::transactions::TxnSystemClient::KeepTransactionAlive(long, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, kudu::MonoDelta) src/kudu/transactions/txn_system_client.cc:320:3
#4 0x7f48e24c62b9 in kudu::transactions::TxnManager::KeepTransactionAlive(long, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, kudu::MonoTime const&) src/kudu/master/txn_manager.cc:238:27
#5 0x7f48e24ca36f in kudu::transactions::TxnManagerServiceImpl::KeepTransactionAlive(kudu::transactions::KeepTransactionAliveRequestPB const*, kudu::transactions::KeepTransactionAliveResponsePB*, kudu::rpc::RpcContext*) src/kudu/master/txn_manager_service.cc:159:42
#6 0x7f48d7224b0e in std::function<void (google::protobuf::Message const*, google::protobuf::Message*, kudu::rpc::RpcContext*)>::operator()(google::protobuf::Message const*, google::protobuf::Message*, kudu::rpc::RpcContext*) const ../../../include/c++/7.5.0/bits/std_function.h:706:14
#7 0x7f48d7223afc in kudu::rpc::GeneratedServiceIf::Handle(kudu::rpc::InboundCall*) src/kudu/rpc/service_if.cc:137:3
#8 0x7f48d7229a9d in kudu::rpc::ServicePool::RunThread() src/kudu/rpc/service_pool.cc:232:15
#9 0x7f48d1290c3a in kudu::Thread::SuperviseThread(void*) src/kudu/util/thread.cc:674:3
#10 0x7f48d3a046da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
#11 0x7f48cd55771e in clone (/lib/x86_64-linux-gnu/libc.so.6+0x12171e)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/kudu/util/monotime.cc:218:10
[1] http://dist-test.cloudera.org/job?job_id=jenkins-slave.1623914260.1110749
Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d
---
M src/kudu/transactions/txn_system_client.cc
M src/kudu/transactions/txn_system_client.h
M src/kudu/util/monotime.cc
3 files changed, 3 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/17611/1
--
To view, visit http://gerrit.cloudera.org:8080/17611
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d
Gerrit-Change-Number: 17611
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 )
Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync()
......................................................................
Patch Set 2:
> > > Sorry I didn't understand. How does pass-by-value fix the UBSAN
> > > warning?
> >
> > The prior version of the method would take a reference to
> >
> > > Sorry I didn't understand. How does pass-by-value fix the UBSAN
> > > warning?
> >
> > Thank you for looking at this.
> >
> > I quickly looked at the code around when putting together this
> > patch and I was under impression that 'timeout' was passed by
> > reference into the callback functor which on its turn was passed
> > into LookupTabletByKey(), so I suspected a functor being run with
> > de-allocated MonoTime.
> >
> > After taking another look, I found turns out the 'timeout' is
> used
> > to compute 'deadline' using 'timeout' right before doing anything
> > else, and then 'deadline' is actually passed around by _value_ in
> > CoordinateTransactionAsync().
> >
> > So yes, it seems this might be a dud fix, but it should not hurt
> in
> > any other way, and we pass MonoDelta by value in many more other
> > cases anyway, so you consider this just as a 'style' fix :)
>
> > ... so you consider this just as a 'style' fix :)
>
> I meant "... so you can consider this to be just a 'style" fix :)"
After another look, it seems the issue here is about passing MonoDelta::FromNanoseconds(std::numeric_limits<int64_t>::max()) as a value for 'timeout' parameter when calling TxnSystemClient::CoordinateTransactionAsync()
--
To view, visit http://gerrit.cloudera.org:8080/17611
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d
Gerrit-Change-Number: 17611
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 25 Jun 2021 21:52:40 +0000
Gerrit-HasComments: No
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17611 )
Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync()
......................................................................
[txn_system_client] MonoDelta by value in CoordinateTransactionAsync()
This patch updates the signature of the
TxnSystemClient::CoordinateTransactionAsync() method to pass MonoDelta
by value. In addition, I added an extra DCHECK() into
MonoTime::AddDelta().
The motivation for this change was seeing the following UBSAN warning
in one of the pre-commit builds [1]:
#0 0x7f48d11e8f3c in kudu::MonoTime::AddDelta(kudu::MonoDelta const&) src/kudu/util/monotime.cc:218:10
#1 0x7f48d11e9eee in kudu::operator+(kudu::MonoTime const&, kudu::MonoDelta const&) src/kudu/util/monotime.cc:333:7
#2 0x7f48e0d846c1 in kudu::transactions::TxnSystemClient::CoordinateTransactionAsync(kudu::tserver::CoordinatorOpPB, kudu::MonoDelta const&, std::function<void (kudu::Status const&)> const&, kudu::tserver::CoordinatorOpResultPB*) src/kudu/transactions/txn_system_client.cc:331:45
#3 0x7f48e0d86f76 in kudu::transactions::TxnSystemClient::KeepTransactionAlive(long, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, kudu::MonoDelta) src/kudu/transactions/txn_system_client.cc:320:3
#4 0x7f48e24c62b9 in kudu::transactions::TxnManager::KeepTransactionAlive(long, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, kudu::MonoTime const&) src/kudu/master/txn_manager.cc:238:27
#5 0x7f48e24ca36f in kudu::transactions::TxnManagerServiceImpl::KeepTransactionAlive(kudu::transactions::KeepTransactionAliveRequestPB const*, kudu::transactions::KeepTransactionAliveResponsePB*, kudu::rpc::RpcContext*) src/kudu/master/txn_manager_service.cc:159:42
#6 0x7f48d7224b0e in std::function<void (google::protobuf::Message const*, google::protobuf::Message*, kudu::rpc::RpcContext*)>::operator()(google::protobuf::Message const*, google::protobuf::Message*, kudu::rpc::RpcContext*) const ../../../include/c++/7.5.0/bits/std_function.h:706:14
#7 0x7f48d7223afc in kudu::rpc::GeneratedServiceIf::Handle(kudu::rpc::InboundCall*) src/kudu/rpc/service_if.cc:137:3
#8 0x7f48d7229a9d in kudu::rpc::ServicePool::RunThread() src/kudu/rpc/service_pool.cc:232:15
#9 0x7f48d1290c3a in kudu::Thread::SuperviseThread(void*) src/kudu/util/thread.cc:674:3
#10 0x7f48d3a046da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
#11 0x7f48cd55771e in clone (/lib/x86_64-linux-gnu/libc.so.6+0x12171e)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/kudu/util/monotime.cc:218:10
[1] http://dist-test.cloudera.org/job?job_id=jenkins-slave.1623914260.1110749
Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d
Reviewed-on: http://gerrit.cloudera.org:8080/17611
Tested-by: Kudu Jenkins
Reviewed-by: Abhishek Chennaka <ac...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/transactions/txn_system_client.cc
M src/kudu/transactions/txn_system_client.h
M src/kudu/util/monotime.cc
3 files changed, 3 insertions(+), 2 deletions(-)
Approvals:
Kudu Jenkins: Verified
Abhishek Chennaka: Looks good to me, but someone else must approve
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/17611
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d
Gerrit-Change-Number: 17611
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 )
Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync()
......................................................................
Patch Set 2:
Sorry I didn't understand. How does pass-by-value fix the UBSAN warning?
--
To view, visit http://gerrit.cloudera.org:8080/17611
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d
Gerrit-Change-Number: 17611
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 25 Jun 2021 19:17:09 +0000
Gerrit-HasComments: No
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 )
Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync()
......................................................................
Patch Set 2:
> Sorry I didn't understand. How does pass-by-value fix the UBSAN
> warning?
The prior version of the method would take a reference to
> Sorry I didn't understand. How does pass-by-value fix the UBSAN
> warning?
Thank you for looking at this.
I quickly looked at the code around when putting together this patch and I was under impression that 'timeout' was passed by reference into the callback functor which on its turn was passed into LookupTabletByKey(), so I suspected a functor being run with de-allocated MonoTime.
After taking another look, I found turns out the 'timeout' is used to compute 'deadline' using 'timeout' right before doing anything else, and then 'deadline' is actually passed around by _value_ in CoordinateTransactionAsync().
So yes, it seems this might be a dud fix, but it should not hurt in any other way, and we pass MonoDelta by value in many more other cases anyway, so you consider this just as a 'style' fix :)
--
To view, visit http://gerrit.cloudera.org:8080/17611
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d
Gerrit-Change-Number: 17611
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 25 Jun 2021 21:24:46 +0000
Gerrit-HasComments: No
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 )
Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync()
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17611
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d
Gerrit-Change-Number: 17611
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 25 Jun 2021 17:52:43 +0000
Gerrit-HasComments: No
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 )
Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync()
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/17611
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d
Gerrit-Change-Number: 17611
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 25 Jun 2021 17:51:51 +0000
Gerrit-HasComments: No
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 )
Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync()
......................................................................
Patch Set 2:
> Patch Set 2:
>
> > > > Sorry I didn't understand. How does pass-by-value fix the UBSAN
> > > > warning?
> > >
> > > The prior version of the method would take a reference to
> > >
> > > > Sorry I didn't understand. How does pass-by-value fix the UBSAN
> > > > warning?
> > >
> > > Thank you for looking at this.
> > >
> > > I quickly looked at the code around when putting together this
> > > patch and I was under impression that 'timeout' was passed by
> > > reference into the callback functor which on its turn was passed
> > > into LookupTabletByKey(), so I suspected a functor being run with
> > > de-allocated MonoTime.
> > >
> > > After taking another look, I found turns out the 'timeout' is
> > used
> > > to compute 'deadline' using 'timeout' right before doing anything
> > > else, and then 'deadline' is actually passed around by _value_ in
> > > CoordinateTransactionAsync().
> > >
> > > So yes, it seems this might be a dud fix, but it should not hurt
> > in
> > > any other way, and we pass MonoDelta by value in many more other
> > > cases anyway, so you consider this just as a 'style' fix :)
> >
> > > ... so you consider this just as a 'style' fix :)
> >
> > I meant "... so you can consider this to be just a 'style" fix :)"
>
> After another look, it seems the issue here is about passing MonoDelta::FromNanoseconds(std::numeric_limits<int64_t>::max()) as a value for 'timeout' parameter when calling TxnSystemClient::CoordinateTransactionAsync()
Ah indeed, I assumed this was a case of the MonoDelta being used in an async function but being destructed before actually being used. Good call-out Bankim.
--
To view, visit http://gerrit.cloudera.org:8080/17611
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d
Gerrit-Change-Number: 17611
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 25 Jun 2021 23:22:53 +0000
Gerrit-HasComments: No
[kudu-CR] [txn system client] MonoDelta by value in CoordinateTransactionAsync()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17611 )
Change subject: [txn_system_client] MonoDelta by value in CoordinateTransactionAsync()
......................................................................
Patch Set 2:
> > Sorry I didn't understand. How does pass-by-value fix the UBSAN
> > warning?
>
> The prior version of the method would take a reference to
>
> > Sorry I didn't understand. How does pass-by-value fix the UBSAN
> > warning?
>
> Thank you for looking at this.
>
> I quickly looked at the code around when putting together this
> patch and I was under impression that 'timeout' was passed by
> reference into the callback functor which on its turn was passed
> into LookupTabletByKey(), so I suspected a functor being run with
> de-allocated MonoTime.
>
> After taking another look, I found turns out the 'timeout' is used
> to compute 'deadline' using 'timeout' right before doing anything
> else, and then 'deadline' is actually passed around by _value_ in
> CoordinateTransactionAsync().
>
> So yes, it seems this might be a dud fix, but it should not hurt in
> any other way, and we pass MonoDelta by value in many more other
> cases anyway, so you consider this just as a 'style' fix :)
> ... so you consider this just as a 'style' fix :)
I meant "... so you can consider this to be just a 'style" fix :)"
--
To view, visit http://gerrit.cloudera.org:8080/17611
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I36ba521a3bb7a4ca42a5dc8d383f5d8b6309154d
Gerrit-Change-Number: 17611
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 25 Jun 2021 21:31:38 +0000
Gerrit-HasComments: No