You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by YvonneIronberg <gi...@git.apache.org> on 2015/08/13 22:29:47 UTC
[GitHub] storm pull request: STORM-976
GitHub user YvonneIronberg opened a pull request:
https://github.com/apache/storm/pull/684
STORM-976
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/YvonneIronberg/storm master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/684.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 #684
----
commit b737c2cceb9611ea10c99450c09fb1a4ad28ab02
Author: YvonneIronberg <yv...@gmail.com>
Date: 2015-08-13T20:24:26Z
A fix for STORM-976.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by YvonneIronberg <gi...@git.apache.org>.
Github user YvonneIronberg commented on a diff in the pull request:
https://github.com/apache/storm/pull/684#discussion_r37039905
--- Diff: storm-core/src/clj/backtype/storm/daemon/supervisor.clj ---
@@ -661,8 +661,8 @@
storm-conf-file (System/getProperty "storm.conf.file")
storm-log-dir (or (System/getProperty "storm.log.dir") (str storm-home file-path-separator "logs"))
storm-conf (read-storm-config)
- storm-log-conf-dir (storm-conf "storm.logback.conf.dir")
- storm-logback-conf-dir (or storm-log-conf-dir (str storm-home file-path-separator "log4j2"))
+ storm-log-conf-dir (storm-conf "storm.log4j2.conf.dir")
--- End diff --
Thanks for the review. Sure. Updated the patch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on a diff in the pull request:
https://github.com/apache/storm/pull/684#discussion_r37129089
--- Diff: bin/storm-config.cmd ---
@@ -83,10 +83,10 @@ if not defined STORM_LOG_DIR (
)
@rem
-@rem retrieve storm.logback.conf.dir from conf file
+@rem retrieve storm.log4j2.conf.dir from conf file
@rem
-"%JAVA%" -client -Dstorm.options= -Dstorm.conf.file= -cp "%CLASSPATH%" backtype.storm.command.config_value storm.logback.conf.dir > %CMD_TEMP_FILE%
+"%JAVA%" -client -Dstorm.options= -Dstorm.conf.file= -cp "%CLASSPATH%" backtype.storm.command.config_value storm.log4j2.conf.dir > %CMD_TEMP_FILE%
--- End diff --
+1 for fixing these things now. Configuration (logs and otherwise) are a big stumbling block for new users. Anything we can do to alleviate such things would be good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/684#discussion_r37024792
--- Diff: storm-core/src/clj/backtype/storm/daemon/supervisor.clj ---
@@ -661,8 +661,8 @@
storm-conf-file (System/getProperty "storm.conf.file")
storm-log-dir (or (System/getProperty "storm.log.dir") (str storm-home file-path-separator "logs"))
storm-conf (read-storm-config)
- storm-log-conf-dir (storm-conf "storm.logback.conf.dir")
- storm-logback-conf-dir (or storm-log-conf-dir (str storm-home file-path-separator "log4j2"))
+ storm-log-conf-dir (storm-conf "storm.log4j2.conf.dir")
--- End diff --
Hey, there. I know you're just updating the existing code, but would you mind adding `"storm.log4j2.conf.dir"` to Config.java and removing the `storm-conf (read-storm-config)` binding?
Both of those lines were added in #373 and went uncaught. Instead of reading from `storm-conf`, just read from `conf`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/684
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by YvonneIronberg <gi...@git.apache.org>.
Github user YvonneIronberg commented on the pull request:
https://github.com/apache/storm/pull/684#issuecomment-131265858
Thanks for the suggestions. Addressed them in the latest commit. logback.xml is converted to an equivalent log4j2.xml according to their docs.
* [logback](http://logback.qos.ch/manual/configuration.html).
* [log4j2 config](https://logging.apache.org/log4j/2.x/manual/configuration.html).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/684#issuecomment-131242201
I looked through the branch and found some I hadn't noticed before:
* [logback.xml](https://github.com/apache/storm/blob/aa308e11685b8b7945f2d4963e9f63dfd322087b/external/flux/flux-core/src/test/resources/logback.xml) file under flux is still there. We could fix this separately in another JIRA since it is external. If we can fix it now, then that is even better.
* [FAQ.md](https://github.com/apache/storm/blob/aa308e11685b8b7945f2d4963e9f63dfd322087b/docs/documentation/FAQ.md) already mentions moving to logback from log4j (1.x), so we should update that one part since we have moved again to log4j2.
* [SECURITY.md](https://github.com/apache/storm/blob/aa308e11685b8b7945f2d4963e9f63dfd322087b/SECURITY.md) also mentions logback in one spot. We could change it or make it generic.
It would be nice to take care of these three as part of this work, otherwise we can create another JIRA Issue to track.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/684#issuecomment-131988403
+1 Looks good to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by YvonneIronberg <gi...@git.apache.org>.
Github user YvonneIronberg commented on a diff in the pull request:
https://github.com/apache/storm/pull/684#discussion_r37103291
--- Diff: storm-core/src/jvm/backtype/storm/Config.java ---
@@ -145,6 +145,12 @@
public static final String STORM_LOCAL_DIR = "storm.local.dir";
public static final Object STORM_LOCAL_DIR_SCHEMA = String.class;
+ /**
+ * A directory that holds configuration files for log4j2.
+ */
+ public static final String STORM_LOG4J2_CONF_DIR = "storm.log4j2.conf.dir";
+ public static final Object STORM_LOG4J2_CONF_DIR_SCHEMA = String.class;
+
--- End diff --
Thanks. Done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/684#discussion_r37092739
--- Diff: storm-core/src/jvm/backtype/storm/Config.java ---
@@ -145,6 +145,12 @@
public static final String STORM_LOCAL_DIR = "storm.local.dir";
public static final Object STORM_LOCAL_DIR_SCHEMA = String.class;
+ /**
+ * A directory that holds configuration files for log4j2.
+ */
+ public static final String STORM_LOG4J2_CONF_DIR = "storm.log4j2.conf.dir";
+ public static final Object STORM_LOG4J2_CONF_DIR_SCHEMA = String.class;
+
--- End diff --
indentation
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by YvonneIronberg <gi...@git.apache.org>.
Github user YvonneIronberg commented on the pull request:
https://github.com/apache/storm/pull/684#issuecomment-132286396
Thanks, @d2r! I've assigned STORM-976 to me on Apache jira.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by YvonneIronberg <gi...@git.apache.org>.
Github user YvonneIronberg commented on the pull request:
https://github.com/apache/storm/pull/684#issuecomment-131470825
Passed Travis CI build.
Changing external/flux/flux-core/src/test/resources/logback.xml to log4j2.xml is in accordance with [STORM-850](https://issues.apache.org/jira/browse/STORM-850) that [changed storm-core/test/resources/logback-test.xml to log4j2-test.xml in a similar way](https://github.com/apache/storm/pull/579/files).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/684#issuecomment-132257022
@YvonneIronberg , I did not see an Apache JIRA user name for you, so I have resolved STORM-976 as Unassigned.
If you would like, share your JIRA user name so that we can assign the STORM-976 to you.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/684#issuecomment-132237904
Looks good to me, +1
Thanks @YvonneIronberg
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-976
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/684#discussion_r37120357
--- Diff: bin/storm-config.cmd ---
@@ -83,10 +83,10 @@ if not defined STORM_LOG_DIR (
)
@rem
-@rem retrieve storm.logback.conf.dir from conf file
+@rem retrieve storm.log4j2.conf.dir from conf file
@rem
-"%JAVA%" -client -Dstorm.options= -Dstorm.conf.file= -cp "%CLASSPATH%" backtype.storm.command.config_value storm.logback.conf.dir > %CMD_TEMP_FILE%
+"%JAVA%" -client -Dstorm.options= -Dstorm.conf.file= -cp "%CLASSPATH%" backtype.storm.command.config_value storm.log4j2.conf.dir > %CMD_TEMP_FILE%
--- End diff --
I was looking around and found other usages of LOCKBACK in the storm-config.cmd file. May as well change those while we are changing the others.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---