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 2019/10/20 07:05:10 UTC
[kudu-CR] util: fix macOS build of thread.cc
Hello Adar Dembo, Hannah Nguyen,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/14516
to review the following change.
Change subject: util: fix macOS build of thread.cc
......................................................................
util: fix macOS build of thread.cc
On macOS builds would fail with:
Undefined symbols for architecture x86_64:
"kudu::EasyJson& kudu::EasyJson::operator=<unsigned long>(unsigned long)", referenced from:
kudu::ThreadMgr::ThreadPathHandler(kudu::WebCallbackRegistry::WebRequest const&, kudu::WebCallbackRegistry::WebResponse*) const in thread.cc.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Rather than instantiating an EasyJson template for unsigned long, which
would be ambiguous, this replaces the usage of size_t with uint64_t.
Change-Id: Iddb61d9a933a9b76ec73fbfd0a4ef555bc27786d
---
M src/kudu/util/thread.cc
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/14516/1
--
To view, visit http://gerrit.cloudera.org:8080/14516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iddb61d9a933a9b76ec73fbfd0a4ef555bc27786d
Gerrit-Change-Number: 14516
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
[kudu-CR] util: fix macOS build of thread.cc
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14516 )
Change subject: util: fix macOS build of thread.cc
......................................................................
util: fix macOS build of thread.cc
On macOS builds would fail with:
Undefined symbols for architecture x86_64:
"kudu::EasyJson& kudu::EasyJson::operator=<unsigned long>(unsigned long)", referenced from:
kudu::ThreadMgr::ThreadPathHandler(kudu::WebCallbackRegistry::WebRequest const&, kudu::WebCallbackRegistry::WebResponse*) const in thread.cc.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Rather than instantiating an EasyJson template for unsigned long, which
would be ambiguous, this replaces the usage of size_t with uint64_t.
Change-Id: Iddb61d9a933a9b76ec73fbfd0a4ef555bc27786d
Reviewed-on: http://gerrit.cloudera.org:8080/14516
Tested-by: Kudu Jenkins
Reviewed-by: Hannah Nguyen <ha...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/util/thread.cc
1 file changed, 1 insertion(+), 1 deletion(-)
Approvals:
Kudu Jenkins: Verified
Hannah Nguyen: Looks good to me, but someone else must approve
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iddb61d9a933a9b76ec73fbfd0a4ef555bc27786d
Gerrit-Change-Number: 14516
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] util: fix macOS build of thread.cc
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14516 )
Change subject: util: fix macOS build of thread.cc
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14516/1/src/kudu/util/thread.cc
File src/kudu/util/thread.cc:
http://gerrit.cloudera.org:8080/#/c/14516/1/src/kudu/util/thread.cc@a430
PS1, Line 430:
> It's not the fact that this map is size_t, but rather the assignment at L45
Yep, on MacOS uint64_t and size_t are different types.
Try this on macOS and Linux:
#include <cstdint>
#include <iostream>
#include <type_traits>
using namespace std;
int main() {
cout << std::is_same<size_t, uint64_t>() << endl;
return 0;
}
--
To view, visit http://gerrit.cloudera.org:8080/14516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddb61d9a933a9b76ec73fbfd0a4ef555bc27786d
Gerrit-Change-Number: 14516
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 21 Oct 2019 02:25:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: fix macOS build of thread.cc
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14516 )
Change subject: util: fix macOS build of thread.cc
......................................................................
Patch Set 2:
> Patch Set 2:
>
> This came up last in commit 5c776b0fa. I thought the solution there made more sense: explicitly cast the size_t into a uint64_t before passing to EasyJson rather. Implicitly casting the result of unordered_map::size() from size_t into uint64_t seems worse to me because what's going on is less obvious.
Ack; I don't think I'll switch approaches unless you feel strongly about it. I don't think this approach is that much less obvious.
--
To view, visit http://gerrit.cloudera.org:8080/14516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddb61d9a933a9b76ec73fbfd0a4ef555bc27786d
Gerrit-Change-Number: 14516
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 21 Oct 2019 05:16:24 +0000
Gerrit-HasComments: No
[kudu-CR] util: fix macOS build of thread.cc
Posted by "Hannah Nguyen (Code Review)" <ge...@cloudera.org>.
Hannah Nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/14516 )
Change subject: util: fix macOS build of thread.cc
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/14516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddb61d9a933a9b76ec73fbfd0a4ef555bc27786d
Gerrit-Change-Number: 14516
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sun, 20 Oct 2019 16:32:07 +0000
Gerrit-HasComments: No
[kudu-CR] util: fix macOS build of thread.cc
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14516 )
Change subject: util: fix macOS build of thread.cc
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14516/1/src/kudu/util/thread.cc
File src/kudu/util/thread.cc:
http://gerrit.cloudera.org:8080/#/c/14516/1/src/kudu/util/thread.cc@a430
PS1, Line 430:
> We do have multiple places in the code that use size_t, why doesn't mac bui
It's not the fact that this map is size_t, but rather the assignment at L450 that breaks the build. We define an EasyJson operator= in util/easy_json.cc for uint32_t and uint64_t, but neither work with size_t.
--
To view, visit http://gerrit.cloudera.org:8080/14516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddb61d9a933a9b76ec73fbfd0a4ef555bc27786d
Gerrit-Change-Number: 14516
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sun, 20 Oct 2019 22:18:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: fix macOS build of thread.cc
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14516 )
Change subject: util: fix macOS build of thread.cc
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddb61d9a933a9b76ec73fbfd0a4ef555bc27786d
Gerrit-Change-Number: 14516
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 21 Oct 2019 02:20:07 +0000
Gerrit-HasComments: No
[kudu-CR] util: fix macOS build of thread.cc
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14516 )
Change subject: util: fix macOS build of thread.cc
......................................................................
Patch Set 2:
This came up last in commit 5c776b0fa. I thought the solution there made more sense: explicitly cast the size_t into a uint64_t before passing to EasyJson rather. Implicitly casting the result of unordered_map::size() from size_t into uint64_t seems worse to me because what's going on is less obvious.
--
To view, visit http://gerrit.cloudera.org:8080/14516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddb61d9a933a9b76ec73fbfd0a4ef555bc27786d
Gerrit-Change-Number: 14516
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 21 Oct 2019 04:48:14 +0000
Gerrit-HasComments: No
[kudu-CR] util: fix macOS build of thread.cc
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14516 )
Change subject: util: fix macOS build of thread.cc
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14516/1/src/kudu/util/thread.cc
File src/kudu/util/thread.cc:
http://gerrit.cloudera.org:8080/#/c/14516/1/src/kudu/util/thread.cc@a430
PS1, Line 430:
We do have multiple places in the code that use size_t, why doesn't mac build fail because of those?
--
To view, visit http://gerrit.cloudera.org:8080/14516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddb61d9a933a9b76ec73fbfd0a4ef555bc27786d
Gerrit-Change-Number: 14516
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sun, 20 Oct 2019 21:44:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: fix macOS build of thread.cc
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14516 )
Change subject: util: fix macOS build of thread.cc
......................................................................
Patch Set 2:
> > This came up last in commit 5c776b0fa. I thought the solution there made more sense: explicitly cast the size_t into a uint64_t before passing to EasyJson rather. Implicitly casting the result of unordered_map::size() from size_t into uint64_t seems worse to me because what's going on is less obvious.
>
> Ack; I don't think I'll switch approaches unless you feel strongly about it. I don't think this approach is that much less obvious.
I don't feel strongly about it.
What would be nice is changing EasyJson so this stops happening. Perhaps if we added size_t overloads to EasyJson functions? Or macOS-only unsigned long overloads?
--
To view, visit http://gerrit.cloudera.org:8080/14516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iddb61d9a933a9b76ec73fbfd0a4ef555bc27786d
Gerrit-Change-Number: 14516
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 21 Oct 2019 06:04:24 +0000
Gerrit-HasComments: No