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 2018/07/14 04:09:59 UTC
[kudu-CR] [logging] fix logging init if linking in kudu client library
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10946
Change subject: [logging] fix logging init if linking in kudu client library
......................................................................
[logging] fix logging init if linking in kudu client library
Updated InitGoogleLoggingSafe() and InitGoogleLoggingSafeBasic() to
get desired effects from calling InitGoogleLoggingSafe() even if
the binary linked with kudu client library.
Prior to this fix, the kudu CLI tool could exit on SIGPIPE when
master/tserver abruptly closed connection. That's because the call
to InitGoogleLoggingSafe() in the main() function of the CLI tool
didn't do anything once InitGoogleLoggingSafeBasic() has been
inadvertently called by the linked-in kudu_client library.
Change-Id: I9438248e7f9699cf14d648160d9d516ab27d6ca2
---
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
2 files changed, 65 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/10946/1
--
To view, visit http://gerrit.cloudera.org:8080/10946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9438248e7f9699cf14d648160d9d516ab27d6ca2
Gerrit-Change-Number: 10946
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [logging] fix logging init if linking in kudu client library
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10946 )
Change subject: [logging] fix logging init if linking in kudu client library
......................................................................
Patch Set 1:
Instead of this fix, could we just change the client library to not call InitGoogleLoggingSafeBasic from a module constructor, but rather to lazily call it on first use of one of the library entry points?
Or as that code suggests, limit the ctor call to the 'exported' client build, since the CLI tool uses the non-exported variant?
--
To view, visit http://gerrit.cloudera.org:8080/10946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9438248e7f9699cf14d648160d9d516ab27d6ca2
Gerrit-Change-Number: 10946
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 16 Jul 2018 19:25:15 +0000
Gerrit-HasComments: No
[kudu-CR] [logging] fix logging init if linking in kudu client library
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10946 )
Change subject: [logging] fix logging init if linking in kudu client library
......................................................................
Patch Set 1:
> Instead of this fix, could we just change the client library to not
> call InitGoogleLoggingSafeBasic from a module constructor, but
> rather to lazily call it on first use of one of the library entry
> points?
>
> Or as that code suggests, limit the ctor call to the 'exported'
> client build, since the CLI tool uses the non-exported variant?
I read those comments about 'multiple entry points' around that c-tor in client.cc and decided to go this way (i.e. the way how PS1 of this patch is built).
Yep, the latter one looks like a good alternative, however I have some concerns with that. If changing the way how it works as you suggested, the problem of 'wrong logging init' can happen again. I.e., imagine someone has linked against the exported library and then called InitGoogleLoggingSafe(), expecting all that extra initialization to happen. However, that would not be the case and that's very frustrating. And the approach in PS1 was built to address exactly that, which I think is the main issue with InitGoogleLoggingSafe() and InitGoogleLoggingSafeBasic() as I see it.
--
To view, visit http://gerrit.cloudera.org:8080/10946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9438248e7f9699cf14d648160d9d516ab27d6ca2
Gerrit-Change-Number: 10946
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 16 Jul 2018 19:42:19 +0000
Gerrit-HasComments: No
[kudu-CR] [logging] fix logging init if linking in kudu client library
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10946 )
Change subject: [logging] fix logging init if linking in kudu client library
......................................................................
Patch Set 1:
> imagine someone has linked against the exported library and then called InitGoogleLoggingSafe(), expecting all that extra initialization to happen
That isn't really possible because the InitGoogleLogging* functions aren't exported APIs and we don't export glog symbols. So, even if the user happens to also use glog, they'll have a different set of glog symbols and state, so there wouldn't be any conflict.
--
To view, visit http://gerrit.cloudera.org:8080/10946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9438248e7f9699cf14d648160d9d516ab27d6ca2
Gerrit-Change-Number: 10946
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 16 Jul 2018 19:46:12 +0000
Gerrit-HasComments: No
[kudu-CR] [logging] fix logging init if linking in kudu client library
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10946 )
Change subject: [logging] fix logging init if linking in kudu client library
......................................................................
Patch Set 1:
> > imagine someone has linked against the exported library and then
> called InitGoogleLoggingSafe(), expecting all that extra
> initialization to happen
>
> That isn't really possible because the InitGoogleLogging* functions
> aren't exported APIs and we don't export glog symbols. So, even if
> the user happens to also use glog, they'll have a different set of
> glog symbols and state, so there wouldn't be any conflict.
All right, that makes sense -- I missed the point about non-exported logging initialization functions. I'll re-do the patch then, thanks.
--
To view, visit http://gerrit.cloudera.org:8080/10946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9438248e7f9699cf14d648160d9d516ab27d6ca2
Gerrit-Change-Number: 10946
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 16 Jul 2018 19:51:09 +0000
Gerrit-HasComments: No
[kudu-CR] [logging] fix logging init if linking in kudu client library
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has abandoned this change. ( http://gerrit.cloudera.org:8080/10946 )
Change subject: [logging] fix logging init if linking in kudu client library
......................................................................
Abandoned
Obsoleted by https://gerrit.cloudera.org/#/c/10956/
--
To view, visit http://gerrit.cloudera.org:8080/10946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I9438248e7f9699cf14d648160d9d516ab27d6ca2
Gerrit-Change-Number: 10946
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>