You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by HeartSaVioR <gi...@git.apache.org> on 2016/03/04 04:46:20 UTC
[GitHub] storm pull request: STORM-1603 Storm UT fails on supervisor test i...
GitHub user HeartSaVioR opened a pull request:
https://github.com/apache/storm/pull/1183
STORM-1603 Storm UT fails on supervisor test in Windows
* supervisor UTs seem not handle file path delimeter properly
* due to absolute-storm-local-dir, we should care Windows specific
* if worker dir path is relative and storm.home is empty, path will be invalid
I think it would affect 1.x and even master, but other tests disturb me verifying test result of supervisor-test.
For example, blobstore UTs are failed on Windows, and it even makes other UTs crashed (so that running test is stopped at the moment).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HeartSaVioR/storm STORM-1603
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1183.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 #1183
----
commit 1b6e91ecaf1804fed718c2997f7c82b8c3738b99
Author: Jungtaek Lim <ka...@gmail.com>
Date: 2016-03-04T03:40:27Z
STORM-1603 Storm UT fails on supervisor test in Windows
* supervisor UTs seem not handle file path delimeter properly
* due to absolute-storm-local-dir, we should care Windows specific
* if worker dir path is relative and storm.home is empty, path will be invalid
----
---
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-1603 Storm UT fails on supervisor test i...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1183#discussion_r55000427
--- Diff: storm-core/test/clj/backtype/storm/supervisor_test.clj ---
@@ -261,8 +260,8 @@
(str "-Dstorm.id=" mock-storm-id)
(str "-Dworker.id=" mock-worker-id)
(str "-Dworker.port=" mock-port)
- "-Dstorm.log.dir=/logs"
- "-Dlog4j.configurationFile=/log4j2/worker.xml"
+ (str "-Dstorm.log.dir=" file-path-separator "logs")
+ (str "-Dlog4j.configurationFile=" file-path-separator "log4j2" file-path-separator "worker.xml")
--- End diff --
Seems like there're many pull requests regarding log4j configuration applied without backporting to 0.10.x. STORM-1463 is just one of these.
I'm wondering we would want to find whole of things and backport to 0.10.x. If we don't want, this PR is just good to go for 0.10.x, and I'll create pull request for 1.x and master.
What do you think?
---
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-1603 Storm UT fails on supervisor test i...
Posted by hustfxj <gi...@git.apache.org>.
Github user hustfxj commented on a diff in the pull request:
https://github.com/apache/storm/pull/1183#discussion_r55001806
--- Diff: storm-core/test/clj/backtype/storm/supervisor_test.clj ---
@@ -261,8 +260,8 @@
(str "-Dstorm.id=" mock-storm-id)
(str "-Dworker.id=" mock-worker-id)
(str "-Dworker.port=" mock-port)
- "-Dstorm.log.dir=/logs"
- "-Dlog4j.configurationFile=/log4j2/worker.xml"
+ (str "-Dstorm.log.dir=" file-path-separator "logs")
+ (str "-Dlog4j.configurationFile=" file-path-separator "log4j2" file-path-separator "worker.xml")
--- End diff --
@HeartSaVioR sorry, I didn't see STORM-1463 before. I think we should find whole of things and fix it. Thank you @HeartSaVioR
---
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-1603 Storm UT fails on supervisor test i...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/1183#issuecomment-193887585
@HeartSaVioR
That depends on how big the diff is. If it's a large diff, I don't think we want to do that. I would much prefer to pull individual patches in to ensure we're not adding features or creating regressions.
This conversation probably belongs on the mailing list, though, so others can weigh in. For now, I think we can merge this and treat backporting fixes to 10.x as a separate issue.
---
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-1603 Storm UT fails on supervisor test i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1183
---
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-1603 Storm UT fails on supervisor test i...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1183#issuecomment-194101069
OK, sent the mail to dev. I'll merge this...
---
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-1603 Storm UT fails on supervisor test i...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1183#discussion_r54999493
--- Diff: storm-core/test/clj/backtype/storm/supervisor_test.clj ---
@@ -261,8 +260,8 @@
(str "-Dstorm.id=" mock-storm-id)
(str "-Dworker.id=" mock-worker-id)
(str "-Dworker.port=" mock-port)
- "-Dstorm.log.dir=/logs"
- "-Dlog4j.configurationFile=/log4j2/worker.xml"
+ (str "-Dstorm.log.dir=" file-path-separator "logs")
+ (str "-Dlog4j.configurationFile=" file-path-separator "log4j2" file-path-separator "worker.xml")
--- End diff --
@hustfxj
STORM-1463 is merged into master (2.x) and 1.x, and this pull request is for 0.10.x.
While this PR can still make unit tests succeed, we may want to backport STORM-1463 to 0.10.x-branch and reflect change to supervisor_test.clj.
---
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-1603 Storm UT fails on supervisor test i...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/1183#issuecomment-193475643
+1. I'm in favor of backporting STORM-1463 (#1004) as well.
---
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-1603 Storm UT fails on supervisor test i...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1183#issuecomment-193545149
@knusbaum STORM-1463 is just one of them.
I can't find other issues which affect this since history for supervisor.clj is gone when moving package.
Do we want to just sync up with supervisor.clj in 1.x-branch (only log related) disregarding the history (finding whole issues and change fix versions)?
---
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-1603 Storm UT fails on supervisor test i...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1183#issuecomment-194076581
@knusbaum
Yes, I'll send the mail to dev mailing list. For now, I'm also in favor of separating issue as `fixing UT` and `catching up change with backporting`.
---
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-1603 Storm UT fails on supervisor test i...
Posted by hustfxj <gi...@git.apache.org>.
Github user hustfxj commented on a diff in the pull request:
https://github.com/apache/storm/pull/1183#discussion_r54991044
--- Diff: storm-core/test/clj/backtype/storm/supervisor_test.clj ---
@@ -261,8 +260,8 @@
(str "-Dstorm.id=" mock-storm-id)
(str "-Dworker.id=" mock-worker-id)
(str "-Dworker.port=" mock-port)
- "-Dstorm.log.dir=/logs"
- "-Dlog4j.configurationFile=/log4j2/worker.xml"
+ (str "-Dstorm.log.dir=" file-path-separator "logs")
+ (str "-Dlog4j.configurationFile=" file-path-separator "log4j2" file-path-separator "worker.xml")
--- End diff --
I think you maybe need fix -Dlog4j.configurationFile as supervisor.clj’s:
log4j-configuration-file (str (if (.startsWith (System/getProperty "os.name") "Windows")
(if (.startsWith storm-log4j2-conf-dir "file:")
storm-log4j2-conf-dir
(str "file:///" storm-log4j2-conf-dir))
storm-log4j2-conf-dir)
Utils/FILE_PATH_SEPARATOR "worker.xml")
---
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.
---