You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by jacobtolar <gi...@git.apache.org> on 2018/08/16 17:03:31 UTC
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
GitHub user jacobtolar opened a pull request:
https://github.com/apache/storm/pull/2806
[STORM-3198] Topology submitters should be able to supply log4j2 conf
This adds a new config setting, `topology.logging.config`, that allows a topology submitter to specify an additional log4j2 config to be used via composite configuration.
When the worker starts the `-Dlog4j.configurationFile` option is modified to include the additional file if requested.
To test, I submitted a topology (FastWordCountTopology):
```
bin/storm jar -c topology.logging.config=classpath:resources/topology_log4j2.xml ../../../../../examples/storm-starter/target/storm-starter-*.jar org.apache.storm.starter.FastWordCountTopology fwc
```
topology_log4j2.xml:
```
<configuration monitorInterval="60" shutdownHook="disable">
<loggers>
<root>
<RegexFilter regex=".*Preparing.*" onMatch="DENY" onMismatch="ACCEPT" />
</root>
<Logger name="org.apache.storm.metric.LoggingMetricsConsumer">
<RegexFilter regex=".*Preparing.*" onMatch="DENY" onMismatch="ACCEPT" />
</Logger>
<Logger name="STDERR">
<RegexFilter regex=".*Preparing.*" onMatch="DENY" onMismatch="ACCEPT" />
</Logger>
<Logger name="STDOUT">
<RegexFilter regex=".*Preparing.*" onMatch="DENY" onMismatch="ACCEPT" />
</Logger>
</loggers>
</configuration>
```
With this child configuration added, the messages about [preparing bolts](https://github.com/apache/storm/blob/26d2f955251c0fa56520f6fe08cb35ef5171f321/storm-client/src/jvm/org/apache/storm/executor/bolt/BoltExecutor.java#L109) no longer appear in the worker log.
I also tested changing simply changing the log level in the child log4j2 config and that also works as expected.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jacobtolar/storm STORM-3198
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2806.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2806
----
commit 28ad8dc77b409339768484839ab6979f42ce07ff
Author: Jacob Tolar <jt...@...>
Date: 2018-08-15T19:53:42Z
[STORM-3198] Topology submitters should be able to supply log4j2 configurations
----
---
[GitHub] storm issue #2806: [STORM-3198] Topology submitters should be able to supply...
Posted by jacobtolar <gi...@git.apache.org>.
Github user jacobtolar commented on the issue:
https://github.com/apache/storm/pull/2806
Yeah, sorry it took me a while to get to fixing it. Hopefully that does it.
---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Posted by jacobtolar <gi...@git.apache.org>.
Github user jacobtolar commented on a diff in the pull request:
https://github.com/apache/storm/pull/2806#discussion_r210768599
--- Diff: storm-server/src/main/java/org/apache/storm/daemon/supervisor/BasicContainer.java ---
@@ -622,6 +620,12 @@ protected String javaCmd(String cmd) {
List<String> classPathParams = getClassPathParams(stormRoot, topoVersion);
List<String> commonParams = getCommonParams();
+ String log4jConfigurationFile = getWorkerLoggingConfigFile();
--- End diff --
No, it can't -- The xml file might be in the topology jar uploaded to the nimbus. That file is not on the classpath if the LogWriter feature is used.
So I guess it could, but getCommonParams would have to take a parameter and get invoked twice. Right now it's just invoked once and the output is used for both LogWriter and Worker jvm startup scripts.
---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2806
---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2806#discussion_r210683327
--- Diff: pom.xml ---
@@ -278,7 +278,7 @@
<netty.version>4.1.25.Final</netty.version>
<sysout-over-slf4j.version>1.0.2</sysout-over-slf4j.version>
<log4j-over-slf4j.version>1.6.6</log4j-over-slf4j.version>
- <log4j.version>2.8.2</log4j.version>
+ <log4j.version>2.11.0</log4j.version>
--- End diff --
Unless we know of a reason not to, we could just bump to the latest version IMO.
---
[GitHub] storm issue #2806: [STORM-3198] Topology submitters should be able to supply...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2806
+1. Please squash to one commit @jacobtolar.
---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Posted by jacobtolar <gi...@git.apache.org>.
Github user jacobtolar commented on a diff in the pull request:
https://github.com/apache/storm/pull/2806#discussion_r210671831
--- Diff: pom.xml ---
@@ -278,7 +278,7 @@
<netty.version>4.1.25.Final</netty.version>
<sysout-over-slf4j.version>1.0.2</sysout-over-slf4j.version>
<log4j-over-slf4j.version>1.6.6</log4j-over-slf4j.version>
- <log4j.version>2.8.2</log4j.version>
+ <log4j.version>2.11.0</log4j.version>
--- End diff --
Note: The update to log4j2-2.11.0 is not strictly necessary. 2.8.2 supports composite configuration. However, there was [a bug](https://issues.apache.org/jira/browse/LOG4J2-2123) in merging filters from child configurations that I got fixed in 2.11.0. If a child configuration added filters and there were none in the parent, the child filter would be ignored.
I would like to add some filters in my topology, so, bumping the version to the one with the fix.
---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2806#discussion_r210682266
--- Diff: storm-client/src/jvm/org/apache/storm/Config.java ---
@@ -705,6 +705,13 @@
*/
@isString(acceptedValues = { "S0", "S1", "S2", "S3" })
public static final String TOPOLOGY_LOGGING_SENSITIVITY = "topology.logging.sensitivity";
+ /**
+ * Log file the user can use to configure Log4j2.
--- End diff --
Nit: Consider mentioning that the configuration file is applied in addition to the regular config file, using these rules https://logging.apache.org/log4j/2.x/manual/configuration.html#CompositeConfiguration.
---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Posted by jacobtolar <gi...@git.apache.org>.
Github user jacobtolar commented on a diff in the pull request:
https://github.com/apache/storm/pull/2806#discussion_r210769170
--- Diff: pom.xml ---
@@ -278,7 +278,7 @@
<netty.version>4.1.25.Final</netty.version>
<sysout-over-slf4j.version>1.0.2</sysout-over-slf4j.version>
<log4j-over-slf4j.version>1.6.6</log4j-over-slf4j.version>
- <log4j.version>2.8.2</log4j.version>
+ <log4j.version>2.11.0</log4j.version>
--- End diff --
not aware of any such reason; done
---
[GitHub] storm issue #2806: [STORM-3198] Topology submitters should be able to supply...
Posted by jacobtolar <gi...@git.apache.org>.
Github user jacobtolar commented on the issue:
https://github.com/apache/storm/pull/2806
Looks like there are a couple unit tests I need to fix. Probably won't get to that till next week.
---
[GitHub] storm issue #2806: [STORM-3198] Topology submitters should be able to supply...
Posted by jacobtolar <gi...@git.apache.org>.
Github user jacobtolar commented on the issue:
https://github.com/apache/storm/pull/2806
rebased to latest master and squashed into one commit
---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2806#discussion_r211484225
--- Diff: storm-server/src/main/java/org/apache/storm/daemon/supervisor/BasicContainer.java ---
@@ -622,6 +620,12 @@ protected String javaCmd(String cmd) {
List<String> classPathParams = getClassPathParams(stormRoot, topoVersion);
List<String> commonParams = getCommonParams();
+ String log4jConfigurationFile = getWorkerLoggingConfigFile();
--- End diff --
Thanks for explaining.
---
[GitHub] storm pull request #2806: [STORM-3198] Topology submitters should be able to...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2806#discussion_r210681943
--- Diff: storm-server/src/main/java/org/apache/storm/daemon/supervisor/BasicContainer.java ---
@@ -622,6 +620,12 @@ protected String javaCmd(String cmd) {
List<String> classPathParams = getClassPathParams(stormRoot, topoVersion);
List<String> commonParams = getCommonParams();
+ String log4jConfigurationFile = getWorkerLoggingConfigFile();
--- End diff --
Nit: Can this go in `getCommonParams`?
---