You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/09/22 03:07:45 UTC
[kudu-CR] c++ client: stop requiring the old gcc ABI
Hello Dan Burkert, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4515
to review the following change.
Change subject: c++ client: stop requiring the old gcc ABI
......................................................................
c++ client: stop requiring the old gcc ABI
With the upgrade to clang 3.9 and the transition to libc++ for TSAN, we can
now safely remove the old gcc ABI guard rail without breaking TSAN builds.
The impact on backwards compatibility is unclear. At least one Kudu vendor
has shipped a client package for Ubuntu Xenial built against the old ABI;
that package will be built against the new ABI going forward. Any
application built against the old ABI will be incompatible with said
package once the ABI changes. But, c++ client consumers are few and far
between, and the major consumer of record (Apache Impala) does not yet
build on Xenial.
Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
---
M .ycm_extra_conf.py
M CMakeLists.txt
M docs/release_notes.adoc
M python/setup.py
M src/kudu/client/client.h
M src/kudu/client/client_samples-test.sh
M src/kudu/client/samples/CMakeLists.txt
M thirdparty/build-thirdparty.sh
8 files changed, 8 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/4515/1
--
To view, visit http://gerrit.cloudera.org:8080/4515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] c++ client: stop requiring the old gcc ABI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: c++ client: stop requiring the old gcc ABI
......................................................................
Patch Set 1:
(1 comment)
> Uploaded patch set 1.
Do you think it's worth adding -Wabi option into the clang options to emit warnings in case of ABI mishaps (plus additional -Wabi-tag for GCC)?
http://gerrit.cloudera.org:8080/#/c/4515/1/docs/release_notes.adoc
File docs/release_notes.adoc:
PS1, Line 165: `C++`
nit: {cpp} could also be an option to deal with special meaning of '+' in AsciiDoctor format.
--
To view, visit http://gerrit.cloudera.org:8080/4515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] c++ client: stop requiring the old gcc ABI
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: c++ client: stop requiring the old gcc ABI
......................................................................
Patch Set 4: Code-Review+2 Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/4515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] c++ client: stop requiring the old gcc ABI
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: c++ client: stop requiring the old gcc ABI
......................................................................
c++ client: stop requiring the old gcc ABI
With the upgrade to clang 3.9 and the transition to libc++ for TSAN, we can
now safely remove the old gcc ABI guard rail without breaking TSAN builds.
The impact on backwards compatibility is immaterial. At least one Kudu
vendor has shipped a client package for Ubuntu 16.04 built against the old
ABI; that package will be built against the new ABI going forward. Any
application built against the old ABI will be incompatible with said
package once the ABI changes. But, c++ client consumers are few and far
between, and the major consumer of record (Apache Impala) does not yet
build on Ubuntu 16.04.
Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
Reviewed-on: http://gerrit.cloudera.org:8080/4515
Reviewed-by: Dan Burkert <da...@cloudera.com>
Tested-by: Dan Burkert <da...@cloudera.com>
---
M .ycm_extra_conf.py
M CMakeLists.txt
M docs/release_notes.adoc
M python/setup.py
M src/kudu/client/client.h
M src/kudu/client/client_samples-test.sh
M src/kudu/client/samples/CMakeLists.txt
M thirdparty/build-thirdparty.sh
8 files changed, 9 insertions(+), 51 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/4515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] c++ client: stop requiring the old gcc ABI
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: c++ client: stop requiring the old gcc ABI
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] c++ client: stop requiring the old gcc ABI
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: c++ client: stop requiring the old gcc ABI
......................................................................
Patch Set 1:
Do you think it's worth adding -Wabi warning flag into the clang options to catch possible ABI incompatibilities?
--
To view, visit http://gerrit.cloudera.org:8080/4515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] c++ client: stop requiring the old gcc ABI
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: c++ client: stop requiring the old gcc ABI
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4515/1//COMMIT_MSG
Commit Message:
PS1, Line 12: The impact on backwards compatibility is unclear.
Isn't it clear? This is a breaking change, but it's not a material break for anyone.
http://gerrit.cloudera.org:8080/#/c/4515/1/docs/release_notes.adoc
File docs/release_notes.adoc:
Line 33
Intentional?
Line 165: - The `C++` client library no longer requires the
This should be in the Kudu 1.1 RN.
PS1, Line 168: Xenial
Could you put this in year/month form? I find that easier as a non-Ubuntu zealot :). And isn't it something like 15.04+ or 15.10+?
--
To view, visit http://gerrit.cloudera.org:8080/4515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] c++ client: stop requiring the old gcc ABI
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4515
to look at the new patch set (#2).
Change subject: c++ client: stop requiring the old gcc ABI
......................................................................
c++ client: stop requiring the old gcc ABI
With the upgrade to clang 3.9 and the transition to libc++ for TSAN, we can
now safely remove the old gcc ABI guard rail without breaking TSAN builds.
The impact on backwards compatibility is immaterial. At least one Kudu
vendor has shipped a client package for Ubuntu 16.04 built against the old
ABI; that package will be built against the new ABI going forward. Any
application built against the old ABI will be incompatible with said
package once the ABI changes. But, c++ client consumers are few and far
between, and the major consumer of record (Apache Impala) does not yet
build on Ubuntu 16.04.
Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
---
M .ycm_extra_conf.py
M CMakeLists.txt
M docs/release_notes.adoc
M python/setup.py
M src/kudu/client/client.h
M src/kudu/client/client_samples-test.sh
M src/kudu/client/samples/CMakeLists.txt
M thirdparty/build-thirdparty.sh
8 files changed, 9 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/4515/2
--
To view, visit http://gerrit.cloudera.org:8080/4515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] c++ client: stop requiring the old gcc ABI
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: c++ client: stop requiring the old gcc ABI
......................................................................
Patch Set 1:
(4 comments)
> Do you think it's worth adding -Wabi option into the clang options
> to emit warnings in case of ABI mishaps (plus additional -Wabi-tag
> for GCC)?
The documentation for -Wabi suggests that it doesn't do anything unless we specify -fabi-version, which we don't currently. That could be something to do in the future (i.e. specify version 9 and then turn on -Wabi).
However, I think it's orthogonal to the "new gcc ABI" that I've been talking about in this patch series, which is an ABI change related to how the STL implements the C++11 standard, not to the C++ standard itself.
http://gerrit.cloudera.org:8080/#/c/4515/1//COMMIT_MSG
Commit Message:
PS1, Line 12: The impact on backwards compatibility is unclear.
> Isn't it clear? This is a breaking change, but it's not a material break f
Alright, will restate as such.
http://gerrit.cloudera.org:8080/#/c/4515/1/docs/release_notes.adoc
File docs/release_notes.adoc:
Line 33
> Intentional?
Whoops, no. Must have been bad conflict resolution on my part.
PS1, Line 165: `C++`
> nit: {cpp} could also be an option to deal with special meaning of '+' in A
Yeah, I don't know why I put it in the 1.0 section. My bad.
Will use {cpp}.
PS1, Line 168: Xenial
> Could you put this in year/month form? I find that easier as a non-Ubuntu
I'll switch to 16.04 instead of Xenial.
Yes, we could include releases as old as 15.10, but practically I only really care about LTS releases.
--
To view, visit http://gerrit.cloudera.org:8080/4515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] c++ client: stop requiring the old gcc ABI
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: c++ client: stop requiring the old gcc ABI
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia06edb8a4699fc842a2072e1edab5cfaa14c4381
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No