[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 16: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad/Catalog/Statestore web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Reviewed-on: http://gerrit.cloudera.org:8080/5792
Reviewed-by: Bharath Vissapragada 
Tested-by: Impala Public Jenkins
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 469 insertions(+), 11 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Bharath Vissapragada: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 16: Code-Review+2

Rebased on top of https://gerrit.cloudera.org/#/c/6264/. Carrying Henry's +2.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 16:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/347/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 15:

Sounds good - I commented on the bugfix, hopefully we can get these both in 
quickly.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 15:

@Henry: The GVO of this patch exposed an unrelated bug that I fixed in another 
CR [1]. I'll GVO this again once that is merged.

[1] https://gerrit.cloudera.org/#/c/6264

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 15:

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/319/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 15:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/319/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 15:

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/316/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 15:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/316/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 15:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/310/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 15:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/311/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 15: Code-Review+2

Thanks Henry. Rebased and increased the page width to 50% for it look good on 
smaller resolutions. Carrying +2.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Hello Henry Robinson,

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

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

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

Change subject: IMPALA-4822: Implement dynamic log level changes
..

IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad/Catalog/Statestore web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 469 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/15
-- 
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 14: Code-Review+2

Discussed offline. I believe the out-of-the-box behaviour hasn't changed.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 40
> This mechanism exists so that we can configure Impala's logging in one plac
> I don't think we should introduce the burden of having them configured 
> separately - is that what this change now implies? 

No. the users can still configure it from a single place 

> Is trace logging off by default? 

TRACE logging is ON/OFF depending on how users configure GLOG (ex: 
JniCatalog/JniFrontend still call GlogAppender.Install() based on --v). 

> We should expect that 95% of deployments won't use this feature for 
> fine-grained control, so the out-of-the-box behaviour has to be good enough 
> for them.

Yes. Like I mentioned above, this patch doesn't break any existing way of 
configuring logging. We still configure it using the central GLOG config.

I think there is some disconnect in my understanding here. This patch doesn't 
break any existing ways per my understanding. May be we can chat offline to 
clear it out. Apologies for the back and forth.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 40
> My understanding is that it should be controlled by log4j on the JVM side r
This mechanism exists so that we can configure Impala's logging in one place, 
and have the behaviour be consistent across FE and BE. I don't think we should 
introduce the burden of having them configured separately - is that what this 
change now implies? Is trace logging off by default?

We should expect that 95% of deployments won't use this feature for 
fine-grained control, so the out-of-the-box behaviour has to be good enough for 
them.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 40
> Right, but the idea is that if we want to turn off trace debugging, for exa
My understanding is that it should be controlled by log4j on the JVM side 
rather than using glog. In this case, if we want to turn off trace debugging, 
we can just "reset Java log levels" and that disables trace logging on the Java 
side itself and the messages won't be passed from JVM to here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 40
> Yes, Interestingly that has always been the behavior (even without this pat
Right, but the idea is that if we want to turn off trace debugging, for 
example, we can use the usual controls to do so (by setting GLOG_v < 3).

What happens with trace log messages now? Are they always printed?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 40
> These come in the way of dynamic log4j dynamic log level changes because th
Does that mean that severities that get mapped to VLOG will always get printed? 
i.e. if we have LOG.debug("something") in the frontend, that will get printed 
at INFO level (per line 55)?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Hello Henry Robinson,

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

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

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

Change subject: IMPALA-4822: Implement dynamic log level changes
..

IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad/Catalog/Statestore web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 469 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/14
-- 
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 13:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 40
> What's the effect of removing these? Where is the VLOG masking done now?
These come in the way of dynamic log4j dynamic log level changes because the 
per class severity could be different from the global log level and hence 
passing the if() check. (severity comes from the logging event)


http://gerrit.cloudera.org:8080/#/c/5792/13/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

PS13, Line 127: set_glog_url =\
  : (self.SET_GLOG_LOGLEVEL_URL + "?glog=foo")
> nit: one line, no parentheses?
Done


http://gerrit.cloudera.org:8080/#/c/5792/13/www/log_level.tmpl
File www/log_level.tmpl:

PS13, Line 21: error
> It might make more sense to print the form if there was an error, so that t
Good point. Done.


PS13, Line 25: width: 600px;
> Why set the width? Does the page look bad if you let it figure out the widt
Yes, it basically takes the full page width and looks awkward. Anyway made it 
30% rather than a specific number. Let me know if you disagree.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 13: Code-Review+2

(4 comments)

Looks pretty good to me.

http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 40
What's the effect of removing these? Where is the VLOG masking done now?


http://gerrit.cloudera.org:8080/#/c/5792/13/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

PS13, Line 127: set_glog_url =\
  : (self.SET_GLOG_LOGLEVEL_URL + "?glog=foo")
nit: one line, no parentheses?


http://gerrit.cloudera.org:8080/#/c/5792/13/www/log_level.tmpl
File www/log_level.tmpl:

PS13, Line 21: error
It might make more sense to print the form if there was an error, so that the 
mistake can be rectified.


PS13, Line 25: width: 600px;
Why set the width? Does the page look bad if you let it figure out the width 
from the parent div?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 13:

Henry, do you have any further comments on this? Thanks.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has uploaded a new patch set (#13).

Change subject: IMPALA-4822: Implement dynamic log level changes
..

IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad/Catalog/Statestore web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 473 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/13
-- 
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 12:

(16 comments)

Fxed a minor bug in the web UI where the setting for java log level changes 
were showing up on the statestore web UI which doesn't have a JVM.

http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

Line 209: 
> remove blank line
Done


http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/util/jni-util.h
File be/src/util/jni-util.h:

PS12, Line 230: the above
> explicitly say LoadJniMethod. Otherwise someone might put a method between 
haha, good point. Changed it.


PS12, Line 234: It seems these
  :   /// must be defined in the header to compile properly.
> can you remove this line (I probably wrote it...)? templates need to be in 
Done


http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

PS12, Line 129: Status SetJavaLogLevel(const TSetJavaLogLevelParams& params, 
string* result) {
  :   RETURN_IF_ERROR(
  :   JniUtil::CallJniMethod(log4j_logger_class_, 
set_log_level_method, params, result));
  :   return Status::OK();
  : }
> Is this called in more than one place? Otherwise, let's just inline it into
Yea that is the only place. I've written this way think it would be more 
readable but I agree it looks redundant.


PS12, Line 143: const Status& status
> What I mean was: rather than constructing a status every time you want to c
Ah sorry misunderstood. Made the change.


PS12, Line 149: SetDisplayResult
> how about 'AddDocumentMember()" ?
That sounds better. Done.


PS12, Line 158: log_getclass->second == nullptr
> Please remove the comparisons to nullptr for strings, and replace with .emp
Done


PS12, Line 215: return
> Maybe make this case an error as well so that user knows what's missing.
Done


PS12, Line 223: std::
> remove
Done


PS12, Line 229: std::
> remove
Done


Line 307: void RegisterLogLevelCallbacks(const string& url, Webserver* 
webserver, bool register_log4j_handlers) {
> long line (consider using git-clang-format)
Done.


PS12, Line 310: set_glog_log
> sorry to nitpick - but set_glog_log seems repetitive. Why not set_glog_leve
Valid point. Changed.


http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/util/logging-support.h
File be/src/util/logging-support.h:

PS12, Line 39: init_log4j
> update
Done


PS12, Line 40: const std::string& url
> is this needed, or is it the same for all callers?
Removed, its the same for all callers.


http://gerrit.cloudera.org:8080/#/c/5792/12/fe/src/main/java/org/apache/impala/util/GlogAppender.java
File fe/src/main/java/org/apache/impala/util/GlogAppender.java:

Line 46: 
> remove blank line
Done


http://gerrit.cloudera.org:8080/#/c/5792/12/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

PS12, Line 86: \
> remove where you have used parentheses
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 12:

> I'm not sure the extra complexity of the UI is worth it.

Without the UI, we'd have to remember how to construct the URLs and what the 
arguments are. The only complexity the UI adds is log_level.tmpl which handles 
presentation of the returned data. Everything else would stay roughly the same 
if we had a command-line-only interface.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 12:

> Marcel, you can make changes like you described just with a URL,
 > see test_web_pages.py which does just that. The UI is nice because
 > then you also have a way of inspecting the state of the system.
 > Allowing mutation without having a way to inspect the state seems
 > bad for usability (and esp. if we expose it to users).

I didn't suggest not allowing inspection of the existing log levels, which can 
also be done via a url.

I'm not sure the extra complexity of the UI is worth it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has uploaded a new patch set (#12).

Change subject: IMPALA-4822: Implement dynamic log level changes
..

IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 475 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/12
-- 
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 7:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

PS7, Line 225:  result.insert(0, "Effective log level: ");
> Mustache supports basic conditionals - you're using one already in the temp
Oh makes sense. Thanks for explaining this.


http://gerrit.cloudera.org:8080/#/c/5792/11/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

PS11, Line 81: ic jmethodID se
> I don't think this is the default, but the originally set value (because th
Done


PS11, Line 96: MethodDescriptor get_log_
> anon namespace
Ouch I thought I already put that. Got confused with the bracketing. Thanks for 
pointing that out.


PS11, Line 217: s_name(log_setclass-
> What information does Status::GetDetail() give you that just passing in the
Any errors in the codepaths of calling jni methods (JniUtil::CallJniMethod()).


PS11, Line 241: ::Parse
> Although I see that this construction compiles in Impala, I can't make it c
Done


PS11, Line 251: 
> just 'classname' or similar. 'log_setclass' is hard to understand.
Done


PS11, Line 252: td::to_string
> just 'level'? Prefer conciseness where you can reasonably do so without com
Done


Line 255:   } else if (args.find("reset_glog_log") != args.end()) {
> No SetErrorMessage()?
Done


PS11, Line 267: 
> empty
Done


PS11, Line 301: 
> if these are not used outside of this module, please put them in the anonym
Done


http://gerrit.cloudera.org:8080/#/c/5792/11/be/src/util/logging-support.h
File be/src/util/logging-support.h:

PS11, Line 22: #include "util/webserver.h"
> not needed: just forward declare the class.
Done


Line 27: struct UTF8;
> While you're here - re-wrap to 90 chars.
Done


Line 39: 
> std::
Done


PS11, Line 39: 
> register_log4j_handlers? 'init_log4j' makes it sound like the log4j subsyst
Done


http://gerrit.cloudera.org:8080/#/c/5792/11/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

PS11, Line 34: // Helper structs for GetJavaLogLevel(), SetJavaLogLevel() and
 : // ResetJavaLogLevel() methods
> This doesn't really help me - comment should say what they're used for.
Done


http://gerrit.cloudera.org:8080/#/c/5792/7/www/log_level.tmpl
File www/log_level.tmpl:

PS7, Line 76: 
> Look at the other templates to see how they do layout?
Thanks for the pointer. Pretty helpful. Moved the code to bootstrap. Looks much 
better.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 1:

Rather than using a web page for making those changes, why not simply do it 
through a URL? (That's how it was handled at Google.)

something like http://.../logv?level= or .../logv_module?=

(I don't care about the actual syntax, that's just an example.)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has uploaded a new patch set (#11).

Change subject: IMPALA-4822: Implement dynamic log level changes
..

IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 483 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/11
-- 
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has uploaded a new patch set (#10).

Change subject: IMPALA-4822: Implement dynamic log level changes
..

IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 483 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/10
-- 
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

PS7, Line 45: TResetJavaLogLevelParams
> I would also settle for comments and / or a better method name if this is t
Henry, I agree with your original comment that passing in these flags for every 
invocation is kind of confusing. To me it seems clearer to keep these defaults 
in the BackendConfig


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

PS7, Line 45: TResetJavaLogLevelParams
> Yes, I'll do it then. Just wanted to confirm that is the ask here.
I would also settle for comments and / or a better method name if this is the 
'right' place to have the parameters.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

PS7, Line 45: TResetJavaLogLevelParams
> Seems easy enough to just keep them in the BackendConfig then
Yes, I'll do it then. Just wanted to confirm that is the ask here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

PS7, Line 45: TResetJavaLogLevelParams
> We already do that [1], just that we don't save them in the frontend anywhe
Seems easy enough to just keep them in the BackendConfig then


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 7:

(29 comments)

http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

Line 37
> where has this gone? where does the definition for Webserver::UrlCallback c
It is now included in logging-support.h


PS7, Line 92:  bind(LogLevelCallBack, _1, _2))
> Everywhere else uses lambdas, please do the same here.
Moved the logic to RegisterLogLevelCallback() as per your suggestion.


http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/backend-gflag-util.h
File be/src/util/backend-gflag-util.h:

Line 29: extern int FLAGS_v_default;
> Placement seems awkward. Maybe we can move thins into logging-support.h/cc 
Done. Moved to logging-support.cc


http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

PS7, Line 79: /
> only // for .cc files
Done


PS7, Line 78: static jclass log4j_logger_class_;
: /// Jni method descriptors corresponding to getLogLevel() and 
setLogLevel() operations.
: static jmethodID get_log_level_method; // 
GlogAppender.getLogLevel()
: static jmethodID set_log_level_method; // 
GlogAppender.setLogLevel()
: static jmethodID reset_log_levels_method; // 
GlogAppender.resetLogLevels()
> Do these need to be in the impala namespace? If not, move to anonymous name
Done


PS7, Line 187: rapidjson
> remove
Done


PS7, Line 197: get_
> don't need to prefix the parameters with 'get'
This was just to make it more readable. Refactored this code.


PS7, Line 207: return
> why not SetErrorMsg() ?
Done


PS7, Line 207: NULL
> prefer nullptr now
Done


PS7, Line 224: if (result == NULL) return;
> how can result be nullptr if it's a stack-allocated string? This doesn't co
Good point. Doesn't it even compile or doesn't it return 1? Anyway, NULL is 
returned by the underlying java calls GetLogLevel() or SetLogLevel() and is set 
in JniUtil::CallJniMethod()


PS7, Line 225:  result.insert(0, "Effective log level: ");
> This kind of presentation logic probably belongs in the template, not here.
I thought about this, but from what I understand, mustache templates are 
logic-less. Am I missing something? Basically this should appear only if we set 
a particular command output.


Line 227:   } else if (args.find("reset_java_log") != args.end()) {
> It might be easier to have several different callbacks:
Yes I was thinking of doing this as the big method seems to be difficult to 
maintain and understand. Made the changes.


Line 243: StringParser::StringToInt(glog_level->second.c_str(),
> use data() instead of c_str()
Removed this part of code and instead added a validator function.


Line 247:   Status s("Bad glog level input. Valid inputs are integers in 
[0-3] range.");
> Even better: in the range [0-3]
Done


Line 247:   Status s("Bad glog level input. Valid inputs are integers in 
[0-3] range.");
> in the [0-3] range
Done


PS7, Line 256: FLAGS_v
> gflags has a SetCommandLineOption() API. Consider using that here instead?
I looked into it and basically does the same thing, like parsing from a string 
and setting the flag. Made the change now, just to be on the safer side like 
avoid races etc. Also looks like gflag lets us register a validator function 
using RegisterFlagValidator() that is called everytime we reset the flag to a 
newer value. I added one so that we don't accidentally set it to a bad value. 
LKM if you'd like any changes in that.


PS7, Line 257: result = "Glog logging level reset to: " + 
std::to_string(FLAGS_v);
> use Substitute()
Done


PS7, Line 260: Value output(result.c_str(), document->GetAllocator());
 :   document->AddMember(display_member, output, 
document->GetAllocator());
> I think it would be clearer to do this inline, and return rather than carry
Done


http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.h
File be/src/util/logging-support.h:

PS7, Line 51: /// Helper method to set the log level of given Java class. It is 
a JNI wrapper around
: /// GlogAppender.setLogLevel().
: Status SetJavaLogLevel(const TSetJavaLogLevelParams& params, 
string* response);
: 
: /// Helper method to get the log level of given Java class. It is 
a JNI wrapper around
: /// GlogAppender.getLogLevel().
: Status GetJavaLogLevel(const TGetJavaLogLevelParams& params, 
string* response);
> Why are these exported? There are no other consumers, right?
Sorry I forgot to clean these up along with ResetJavaLogLevel. Removed.


Line 62: void LogLevelCallBack(const Webserver::ArgumentMap& args, 
rapidjson::Document* document);
> How about "RegisterLogLevelCallback(string url, Webserver*)" ?
Good idea. That cleans up the 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 7:

(22 comments)

Took a look, had some comments.

http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

Line 37
where has this gone? where does the definition for Webserver::UrlCallback come 
from now? It's better to include-what-you-use.


PS7, Line 92:  bind(LogLevelCallBack, _1, _2))
Everywhere else uses lambdas, please do the same here.


http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

PS7, Line 79: /
only // for .cc files


PS7, Line 78: static jclass log4j_logger_class_;
: /// Jni method descriptors corresponding to getLogLevel() and 
setLogLevel() operations.
: static jmethodID get_log_level_method; // 
GlogAppender.getLogLevel()
: static jmethodID set_log_level_method; // 
GlogAppender.setLogLevel()
: static jmethodID reset_log_levels_method; // 
GlogAppender.resetLogLevels()
Do these need to be in the impala namespace? If not, move to anonymous 
namespace.


PS7, Line 187: rapidjson
remove


PS7, Line 197: get_
don't need to prefix the parameters with 'get'


PS7, Line 207: return
why not SetErrorMsg() ?


PS7, Line 207: NULL
prefer nullptr now


PS7, Line 224: if (result == NULL) return;
how can result be nullptr if it's a stack-allocated string? This doesn't 
compile for me:

std::string foo;
if (foo == nullptr) return 1;


PS7, Line 225:  result.insert(0, "Effective log level: ");
This kind of presentation logic probably belongs in the template, not here.


Line 227:   } else if (args.find("reset_java_log") != args.end()) {
It might be easier to have several different callbacks:

  /reset_java_log
  /set_java_log?class=foo=2

to help break this method up. They can all still use the same template.


Line 247:   Status s("Bad glog level input. Valid inputs are integers in 
[0-3] range.");
> in the [0-3] range
Even better: in the range [0-3]


PS7, Line 256: FLAGS_v
gflags has a SetCommandLineOption() API. Consider using that here instead?


PS7, Line 257: result = "Glog logging level reset to: " + 
std::to_string(FLAGS_v);
use Substitute()


PS7, Line 260: Value output(result.c_str(), document->GetAllocator());
 :   document->AddMember(display_member, output, 
document->GetAllocator());
I think it would be clearer to do this inline, and return rather than carry 
result and display_member through the method.

Then you can get rid of the  'else's everywhere.


http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.h
File be/src/util/logging-support.h:

PS7, Line 51: /// Helper method to set the log level of given Java class. It is 
a JNI wrapper around
: /// GlogAppender.setLogLevel().
: Status SetJavaLogLevel(const TSetJavaLogLevelParams& params, 
string* response);
: 
: /// Helper method to get the log level of given Java class. It is 
a JNI wrapper around
: /// GlogAppender.getLogLevel().
: Status GetJavaLogLevel(const TGetJavaLogLevelParams& params, 
string* response);
Why are these exported? There are no other consumers, right?


Line 62: void LogLevelCallBack(const Webserver::ArgumentMap& args, 
rapidjson::Document* document);
How about "RegisterLogLevelCallback(string url, Webserver*)" ?

Then you don't need to worry about getting the rapidjson types declared.


Line 66: void SetErrorMessage(const Status& status, const char* member,
No need for this to be declared in the header.


http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

PS7, Line 45: TResetJavaLogLevelParams
Reset implies 'return to defaults'. So it's confusing that this takes 
parameters - are those the defaults?


http://gerrit.cloudera.org:8080/#/c/5792/7/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

PS7, Line 56: +
it might be easier to read to wrap the string in parentheses:

  get_loglevel_url = ("http://localhost:25000/log_level?;
"getclass=org.apache.impala.catalog.HdfsTable_java_log=...")


http://gerrit.cloudera.org:8080/#/c/5792/7/www/log_level.tmpl
File www/log_level.tmpl:

PS7, Line 32: b
use  instead of 


PS7, Line 76: 
Is there any way not to use tables for layout in this file? They are hard to 
maintain.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 6:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5792/6/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 83: static const string log_inputs[] = {"get_java_log", "set_java_log", 
"reset_java_log",
> Sorry, I didn't realize the log_inputs and the display_members were differe
Changes undone. Renamed a little as discussed offline.


http://gerrit.cloudera.org:8080/#/c/5792/6/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

Line 43: """ Helper method that polls a given url and asserts the return 
code is ok and
> nit: remove space before "Helper"
Done


Line 48:   def test_log_level_callback(self):
> test_log_level
Done


Line 50: malformed inputs"""
> Does not test that the log level modifications are actually in effect.
Done


Line 59: # Set the log level of a class to TRACE anc confirm the setting is 
in place
> typo: and
Done


Line 67: self.get_and_check_status(get_loglevel_url, "TRACE")
> also check a different class and make sure it's still at DEBUG
Done


Line 75: self.get_and_check_status(get_loglevel_url, "DEBUG")
> also add tests for malformed inputs, e.g., a class that does not exist, emp
Added a few more tests. Also, per my understanding, in log4j, there is no such 
thing as a class that does not exists. It accepts any class that we input (and 
sets a log level), since we can actually load them at runtime, if it is not 
already loaded.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has uploaded a new patch set (#7).

Change subject: IMPALA-4822: Implement dynamic log level changes
..

IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 443 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/7
-- 
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 6:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5792/6/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 83: static const string log_inputs[] = {"get_java_log", "set_java_log", 
"reset_java_log",
Sorry, I didn't realize the log_inputs and the display_members were different 
strings, it was easier to read the way it was before.


http://gerrit.cloudera.org:8080/#/c/5792/6/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

Line 43: """ Helper method that polls a given url and asserts the return 
code is ok and
nit: remove space before "Helper"


Line 48:   def test_log_level_callback(self):
test_log_level


Line 50: malformed inputs"""
Does not test that the log level modifications are actually in effect.


Line 59: # Set the log level of a class to TRACE anc confirm the setting is 
in place
typo: and


Line 67: self.get_and_check_status(get_loglevel_url, "TRACE")
also check a different class and make sure it's still at DEBUG


Line 75: self.get_and_check_status(get_loglevel_url, "DEBUG")
also add tests for malformed inputs, e.g., a class that does not exist, empty 
strings, garbage log levels, etc.

same for the glog tests

there are some tests below, but I think some cases are missing


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has uploaded a new patch set (#6).

Change subject: IMPALA-4822: Implement dynamic log level changes
..

IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 415 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/6
-- 
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 5:

(9 comments)

webserver-test.cc seems to be more unit-testing the WebServer class and I don't 
think we can instantiate the JNI and test the actual code paths. So, I added 
new e-e tests to the patch. Please let me know if you think we should add more 
variants.

http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/backend-gflag-util.h
File be/src/util/backend-gflag-util.h:

Line 27: /// debugging, so we save the original value here incase we need to 
restore
> in case (two words)
Done


http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 77: jclass log4j_logger_class_;
> no "_" suffix since not private class members; make these static
Done


Line 196:   if (args.find("get_java_log") != args.end()) {
> make these strings constants like:
Done. Refactored it a little.


Line 212: logging_level == args.end() || logging_level->second == NULL) 
return;
> use {} for multi-line ifs
Done


http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/logging-support.h
File be/src/util/logging-support.h:

Line 25: namespace rapidjson {
> Ok to use the include if you prefer that solution. Either wfm.
I'm fine with this too.


Line 43: /// the Java log4j log messages to be forwarded to Glog.
> update comment to reflect that it loads JNI classes/methods to support dyna
Done


http://gerrit.cloudera.org:8080/#/c/5792/5/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

Line 34: // Helper structs for GetLogLevel(), SetLogLevel() and ResetLogLevel()
> update
Done


http://gerrit.cloudera.org:8080/#/c/5792/5/www/log_level.tmpl
File www/log_level.tmpl:

Line 27:   Log: 
> Pretty sure org.apache.foo won't be clear to users. I suggest using a full,
Done


Line 41:   Log: 
> use a real class name
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 5:

(9 comments)

Since we're accepting user input it would be great to have unit tests. Checking 
that the correct logging level is actually in effect might be a little tricky. 
I'm more thinking along the lines of checking garbage user input, checking that 
settings are reflected and can be reset, etc. Basic validation. I think you 
should be able to do something similar to webserver-test.cc.

http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/backend-gflag-util.h
File be/src/util/backend-gflag-util.h:

Line 27: /// debugging, so we save the original value here incase we need to 
restore
in case (two words)


http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 77: jclass log4j_logger_class_;
no "_" suffix since not private class members; make these static


Line 196:   if (args.find("get_java_log") != args.end()) {
make these strings constants like:

static const char* GET_JAVA_LOG = "get_java_log";
etc.


Line 212: logging_level == args.end() || logging_level->second == NULL) 
return;
use {} for multi-line ifs


http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/logging-support.h
File be/src/util/logging-support.h:

Line 25: namespace rapidjson {
Ok to use the include if you prefer that solution. Either wfm.


Line 43: /// the Java log4j log messages to be forwarded to Glog.
update comment to reflect that it loads JNI classes/methods to support dynamic 
log level changes


http://gerrit.cloudera.org:8080/#/c/5792/5/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

Line 34: // Helper structs for GetLogLevel(), SetLogLevel() and ResetLogLevel()
update


http://gerrit.cloudera.org:8080/#/c/5792/5/www/log_level.tmpl
File www/log_level.tmpl:

Line 27:   Log: 
Pretty sure org.apache.foo won't be clear to users. I suggest using a full, 
valid class name like org.apache.impala.analysis.Analyzer


Line 41:   Log: 
use a real class name


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has uploaded a new patch set (#5).

Change subject: IMPALA-4822: Implement dynamic log level changes
..

IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
A www/log_level.tmpl
11 files changed, 343 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 1:

(2 comments)

PS4 implements dynamic logging changes to the backend (equivalent to setting 
--v on the fly) and also the "reset" functionality for both log4j and the 
backend. That resets the logging state as per the user defined values at the 
startup.

Also, I'm looking for some suggestions on cosmetic changes to the log_level web 
end point. As of now it works, but looks pretty basic (like it is designed by 
someone who just learnt HTML :D). So that needs to be improved as well.

http://gerrit.cloudera.org:8080/#/c/5792/1/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 26: #include "rpc/jni-thrift-util.h"
> names.h basically has a bunch of common "using" directives, so is more appr
Thanks for explaining.


http://gerrit.cloudera.org:8080/#/c/5792/1/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

Line 35: struct TGetLogLevelParams {
> Agree that there is additional bookeeping. I'm generally of the opinion tha
I agree with your point that having a visual feedback is much more easy to the 
end user. For now I've implemented the "reset" functionality. Still need to 
look into log4j if there is a way to get all the overrides.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has uploaded a new patch set (#4).

Change subject: IMPALA-4822: Implement dynamic log level changes
..

IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
A www/log_level.tmpl
11 files changed, 345 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson