You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2017/09/14 20:09:09 UTC

[Impala-ASF-CR] IMPALA-5174: Suppress kudu flags that aren't relevant to Impala 

Sailesh Mukil has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8074

Change subject: IMPALA-5174: Suppress kudu flags that aren't relevant to Impala
......................................................................

IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

Importing Kudu's util/, security/ and rpc/ code into the
Impala code base results in Kudu's gflags showing up in the log
files and the /varz webpages, for all the gflags that belong to
Kudu files that are #include-d in Impala code.

Since we don't want these flags to be user configurable, we don't
want them to be visible. The following commit earlier included a
gflags patch that added a DEFINE_*_hidden() macro that allows us to
do just this:
https://github.com/apache/incubator-impala/commit/d1910a39fcc50ce211b95c3552c0c90b4bc37bbd

This patch was done by running the following sed command:

find $IMPALA_HOME/be/src/kudu -type f | xargs sed -i 's/^\(DEFINE_.*\)\((.*\)/\1_hidden\2/g'

There were a couple non-related macros that also got changed:
DEFINE_validator() and DEFINE_STATIC_THREAD_LOCAL(), which were
manually changed back.

Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
---
M be/src/kudu/rpc/acceptor_pool.cc
M be/src/kudu/rpc/messenger.cc
M be/src/kudu/rpc/negotiation-test.cc
M be/src/kudu/rpc/negotiation.cc
M be/src/kudu/rpc/outbound_call.cc
M be/src/kudu/rpc/reactor.cc
M be/src/kudu/rpc/result_tracker.cc
M be/src/kudu/rpc/rpc-bench.cc
M be/src/kudu/rpc/rpc_stub-test.cc
M be/src/kudu/rpc/rpcz_store.cc
M be/src/kudu/rpc/server_negotiation.cc
M be/src/kudu/rpc/service_if.cc
M be/src/kudu/rpc/service_queue-test.cc
M be/src/kudu/rpc/transfer.cc
M be/src/kudu/security/tls_context.cc
M be/src/kudu/security/token_signer.cc
M be/src/kudu/util/cache.cc
M be/src/kudu/util/debug/trace_event_impl.cc
M be/src/kudu/util/env_posix.cc
M be/src/kudu/util/env_util.cc
M be/src/kudu/util/file_cache-stress-test.cc
M be/src/kudu/util/file_cache.cc
M be/src/kudu/util/flag_tags-test.cc
M be/src/kudu/util/flag_validators-test.cc
M be/src/kudu/util/flags-test.cc
M be/src/kudu/util/flags.cc
M be/src/kudu/util/kernel_stack_watchdog.cc
M be/src/kudu/util/logging.cc
M be/src/kudu/util/maintenance_manager.cc
M be/src/kudu/util/memory/arena-test.cc
M be/src/kudu/util/memory/memory.cc
M be/src/kudu/util/metrics.cc
M be/src/kudu/util/minidump.cc
M be/src/kudu/util/mt-hdr_histogram-test.cc
M be/src/kudu/util/mt-metrics-test.cc
M be/src/kudu/util/mutex.cc
M be/src/kudu/util/net/dns_resolver.cc
M be/src/kudu/util/net/net_util.cc
M be/src/kudu/util/net/socket.cc
M be/src/kudu/util/nvm_cache.cc
M be/src/kudu/util/process_memory.cc
M be/src/kudu/util/spinlock_profiling.cc
M be/src/kudu/util/striped64-test.cc
M be/src/kudu/util/test_main.cc
M be/src/kudu/util/test_util.cc
45 files changed, 117 insertions(+), 117 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/8074/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8074
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>

[Impala-ASF-CR] IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8074 )

Change subject: IMPALA-5174: Suppress kudu flags that aren't relevant to Impala
......................................................................


Patch Set 2: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/8074
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Gerrit-Change-Number: 8074
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Oct 2017 08:26:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8074 )

Change subject: IMPALA-5174: Suppress kudu flags that aren't relevant to Impala
......................................................................


Patch Set 2:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1296/


-- 
To view, visit http://gerrit.cloudera.org:8080/8074
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Gerrit-Change-Number: 8074
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Oct 2017 04:14:59 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8074 )

Change subject: IMPALA-5174: Suppress kudu flags that aren't relevant to Impala
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8074/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8074/1//COMMIT_MSG@12
PS1, Line 12:  #include-d in Impala code
Do we know why this is the case ? I don't see DECLARE_FLAG() in all header files.



-- 
To view, visit http://gerrit.cloudera.org:8080/8074
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Gerrit-Change-Number: 8074
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Oct 2017 19:50:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8074 )

Change subject: IMPALA-5174: Suppress kudu flags that aren't relevant to Impala
......................................................................


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8074/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8074/1//COMMIT_MSG@12
PS1, Line 12:  #include-d in Impala code
> Do we know why this is the case ? I don't see DECLARE_FLAG() in all header 
Only gflags in .cc files which aren't dead code will show up, right ? Please clarify this comment. Simply #include a header file doesn't mean the gflags defined in the .cc file corresponding to the .h file will be included, right ?



-- 
To view, visit http://gerrit.cloudera.org:8080/8074
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Gerrit-Change-Number: 8074
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Oct 2017 00:13:58 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5174: Suppress kudu flags that aren't relevant to Impala
......................................................................


Patch Set 1: Code-Review+1

I think this makes sense. At some point, should we add a test that tracks when parameters are added or removed from the UI? It seems like something that could happen inadvertently. It would add some overhead to adding/removing params.

-- 
To view, visit http://gerrit.cloudera.org:8080/8074
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8074 )

Change subject: IMPALA-5174: Suppress kudu flags that aren't relevant to Impala
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

Rebase, carry +2.

http://gerrit.cloudera.org:8080/#/c/8074/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8074/1//COMMIT_MSG@12
PS1, Line 12:  in some way used by Impal
> Only gflags in .cc files which aren't dead code will show up, right ? Pleas
Yes, that's right. That's what I found in my experiments. I've updated the commit message.



-- 
To view, visit http://gerrit.cloudera.org:8080/8074
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Gerrit-Change-Number: 8074
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Oct 2017 04:14:38 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Joe McDonnell, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/8074

to look at the new patch set (#2).

Change subject: IMPALA-5174: Suppress kudu flags that aren't relevant to Impala
......................................................................

IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

Importing Kudu's util/, security/ and rpc/ code into the
Impala code base results in Kudu's gflags showing up in the log
files and the /varz webpages, for all the gflags that belong to
Kudu files that are in some way used by Impala.

For eg: If in Impala, we include kudu/rpc/messenger.h and use one
of its classes whose method definitions live in the corresponding
messenger.cc file, all the flags DEFINEd in the messenger.cc file
will show up in the Impala logs. If we do not use any code in a .cc
file, the flags that are DEFINEd in that file do not show up (this
is probably due to the compiler not running macros in files that
have only dead code).

Since we don't want these flags to be user configurable, we don't
want them to be visible. The following commit earlier included a
gflags patch that added a DEFINE_*_hidden() macro that allows us to
do just this:
https://github.com/apache/incubator-impala/commit/d1910a39fcc50ce211b95c3552c0c90b4bc37bbd

This patch was done by running the following sed command:

find $IMPALA_HOME/be/src/kudu -type f | xargs sed -i 's/^\(DEFINE_.*\)\((.*\)/\1_hidden\2/g'

There were a couple non-related macros that also got changed:
DEFINE_validator() and DEFINE_STATIC_THREAD_LOCAL(), which were
manually changed back.

Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
---
M be/src/kudu/rpc/acceptor_pool.cc
M be/src/kudu/rpc/messenger.cc
M be/src/kudu/rpc/negotiation-test.cc
M be/src/kudu/rpc/negotiation.cc
M be/src/kudu/rpc/outbound_call.cc
M be/src/kudu/rpc/reactor.cc
M be/src/kudu/rpc/result_tracker.cc
M be/src/kudu/rpc/rpc-bench.cc
M be/src/kudu/rpc/rpc_stub-test.cc
M be/src/kudu/rpc/rpcz_store.cc
M be/src/kudu/rpc/server_negotiation.cc
M be/src/kudu/rpc/service_if.cc
M be/src/kudu/rpc/service_queue-test.cc
M be/src/kudu/rpc/transfer.cc
M be/src/kudu/security/tls_context.cc
M be/src/kudu/security/token_signer.cc
M be/src/kudu/util/cache.cc
M be/src/kudu/util/debug/trace_event_impl.cc
M be/src/kudu/util/env_posix.cc
M be/src/kudu/util/env_util.cc
M be/src/kudu/util/file_cache-stress-test.cc
M be/src/kudu/util/file_cache.cc
M be/src/kudu/util/flag_tags-test.cc
M be/src/kudu/util/flag_validators-test.cc
M be/src/kudu/util/flags-test.cc
M be/src/kudu/util/flags.cc
M be/src/kudu/util/kernel_stack_watchdog.cc
M be/src/kudu/util/logging.cc
M be/src/kudu/util/maintenance_manager.cc
M be/src/kudu/util/memory/arena-test.cc
M be/src/kudu/util/memory/memory.cc
M be/src/kudu/util/metrics.cc
M be/src/kudu/util/minidump.cc
M be/src/kudu/util/mt-hdr_histogram-test.cc
M be/src/kudu/util/mt-metrics-test.cc
M be/src/kudu/util/mutex.cc
M be/src/kudu/util/net/dns_resolver.cc
M be/src/kudu/util/net/net_util.cc
M be/src/kudu/util/net/socket.cc
M be/src/kudu/util/nvm_cache.cc
M be/src/kudu/util/process_memory.cc
M be/src/kudu/util/spinlock_profiling.cc
M be/src/kudu/util/striped64-test.cc
M be/src/kudu/util/test_main.cc
M be/src/kudu/util/test_util.cc
45 files changed, 117 insertions(+), 117 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/8074/2
-- 
To view, visit http://gerrit.cloudera.org:8080/8074
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Gerrit-Change-Number: 8074
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8074 )

Change subject: IMPALA-5174: Suppress kudu flags that aren't relevant to Impala
......................................................................

IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

Importing Kudu's util/, security/ and rpc/ code into the
Impala code base results in Kudu's gflags showing up in the log
files and the /varz webpages, for all the gflags that belong to
Kudu files that are in some way used by Impala.

For eg: If in Impala, we include kudu/rpc/messenger.h and use one
of its classes whose method definitions live in the corresponding
messenger.cc file, all the flags DEFINEd in the messenger.cc file
will show up in the Impala logs. If we do not use any code in a .cc
file, the flags that are DEFINEd in that file do not show up (this
is probably due to the compiler not running macros in files that
have only dead code).

Since we don't want these flags to be user configurable, we don't
want them to be visible. The following commit earlier included a
gflags patch that added a DEFINE_*_hidden() macro that allows us to
do just this:
https://github.com/apache/incubator-impala/commit/d1910a39fcc50ce211b95c3552c0c90b4bc37bbd

This patch was done by running the following sed command:

find $IMPALA_HOME/be/src/kudu -type f | xargs sed -i 's/^\(DEFINE_.*\)\((.*\)/\1_hidden\2/g'

There were a couple non-related macros that also got changed:
DEFINE_validator() and DEFINE_STATIC_THREAD_LOCAL(), which were
manually changed back.

Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Reviewed-on: http://gerrit.cloudera.org:8080/8074
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/kudu/rpc/acceptor_pool.cc
M be/src/kudu/rpc/messenger.cc
M be/src/kudu/rpc/negotiation-test.cc
M be/src/kudu/rpc/negotiation.cc
M be/src/kudu/rpc/outbound_call.cc
M be/src/kudu/rpc/reactor.cc
M be/src/kudu/rpc/result_tracker.cc
M be/src/kudu/rpc/rpc-bench.cc
M be/src/kudu/rpc/rpc_stub-test.cc
M be/src/kudu/rpc/rpcz_store.cc
M be/src/kudu/rpc/server_negotiation.cc
M be/src/kudu/rpc/service_if.cc
M be/src/kudu/rpc/service_queue-test.cc
M be/src/kudu/rpc/transfer.cc
M be/src/kudu/security/tls_context.cc
M be/src/kudu/security/token_signer.cc
M be/src/kudu/util/cache.cc
M be/src/kudu/util/debug/trace_event_impl.cc
M be/src/kudu/util/env_posix.cc
M be/src/kudu/util/env_util.cc
M be/src/kudu/util/file_cache-stress-test.cc
M be/src/kudu/util/file_cache.cc
M be/src/kudu/util/flag_tags-test.cc
M be/src/kudu/util/flag_validators-test.cc
M be/src/kudu/util/flags-test.cc
M be/src/kudu/util/flags.cc
M be/src/kudu/util/kernel_stack_watchdog.cc
M be/src/kudu/util/logging.cc
M be/src/kudu/util/maintenance_manager.cc
M be/src/kudu/util/memory/arena-test.cc
M be/src/kudu/util/memory/memory.cc
M be/src/kudu/util/metrics.cc
M be/src/kudu/util/minidump.cc
M be/src/kudu/util/mt-hdr_histogram-test.cc
M be/src/kudu/util/mt-metrics-test.cc
M be/src/kudu/util/mutex.cc
M be/src/kudu/util/net/dns_resolver.cc
M be/src/kudu/util/net/net_util.cc
M be/src/kudu/util/net/socket.cc
M be/src/kudu/util/nvm_cache.cc
M be/src/kudu/util/process_memory.cc
M be/src/kudu/util/spinlock_profiling.cc
M be/src/kudu/util/striped64-test.cc
M be/src/kudu/util/test_main.cc
M be/src/kudu/util/test_util.cc
45 files changed, 117 insertions(+), 117 deletions(-)

Approvals:
  Sailesh Mukil: Looks good to me, approved
  Impala Public Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/8074
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Gerrit-Change-Number: 8074
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>