You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by Renkai <gi...@git.apache.org> on 2015/09/06 05:52:20 UTC
[GitHub] storm pull request: [STORM-1005] set storm.local.dir relative valu...
GitHub user Renkai opened a pull request:
https://github.com/apache/storm/pull/722
[STORM-1005] set storm.local.dir relative values relative to storm.home rather tha…
Solve the same problem with
https://github.com/apache/storm/pull/713
but another way as @HeartSaVioR advised.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Renkai/storm storm-1005-relative
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/722.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 #722
----
commit 180c51d0cf1017c872e87845cc356b8803e978cf
Author: renkai <ga...@gmail.com>
Date: 2015-09-06T03:47:02Z
set storm.local.dir relative values relative to storm.home rather than where user run storm command
----
---
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-1005] set storm.local.dir relative valu...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/722#discussion_r39817156
--- Diff: storm-core/test/clj/backtype/storm/config_test.clj ---
@@ -168,3 +168,24 @@
(catch Exception e e)))))
(is (thrown-cause? java.lang.IllegalArgumentException
(.validateField validator "test" 42)))))))
+
+(deftest test-absolute-storm-local-dir
+ (let [storm-home-key "storm.home"
+ old-storm-home (System/getProperty storm-home-key)
+ conf-relative {STORM-LOCAL-DIR "storm-local"}
+ conf-absolute {STORM-LOCAL-DIR
+ (if on-windows?
+ "C:\\storm-local"
+ "/var/storm-local")}]
+ (try
+ (System/setProperty storm-home-key (if on-windows? "C:\\storm-home" "/usr/local/storm-home"))
--- End diff --
@Renkai
Unfortunately I found this PR affects other tests randomly - supervisor tests are failing.
Please see https://travis-ci.org/apache/storm/jobs/79231189.
Could we just use current storm.home?
---
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-1005] set storm.local.dir relative valu...
Posted by Renkai <gi...@git.apache.org>.
Github user Renkai commented on the pull request:
https://github.com/apache/storm/pull/722#issuecomment-141328715
@HeartSaVioR It's fine to leave default `storm.home` to test,I manually set it before because I'm not sure if the value is already set when the test code was running.Now I remove the code which may cause environment changing.
---
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-1005] set storm.local.dir relative valu...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/722#issuecomment-141256305
@Renkai
Sorry for the delay. I intended to wait for other committers to review, but since no one is reviewing I'll take care of it.
---
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-1005] set storm.local.dir relative valu...
Posted by Renkai <gi...@git.apache.org>.
Github user Renkai commented on the pull request:
https://github.com/apache/storm/pull/722#issuecomment-138449623
@HeartSaVioR I hope the unit test is good enough,but I don't have a windows environment to test the code.It seems travis-ci do not have windows environment,too.
---
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-1005] set storm.local.dir relative valu...
Posted by Renkai <gi...@git.apache.org>.
Github user Renkai commented on the pull request:
https://github.com/apache/storm/pull/722#issuecomment-140972231
Would you like to merge these commits or they need someone else to review?
---
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-1005] set storm.local.dir relative valu...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/722#discussion_r39817305
--- Diff: storm-core/test/clj/backtype/storm/config_test.clj ---
@@ -168,3 +168,24 @@
(catch Exception e e)))))
(is (thrown-cause? java.lang.IllegalArgumentException
(.validateField validator "test" 42)))))))
+
+(deftest test-absolute-storm-local-dir
+ (let [storm-home-key "storm.home"
+ old-storm-home (System/getProperty storm-home-key)
+ conf-relative {STORM-LOCAL-DIR "storm-local"}
+ conf-absolute {STORM-LOCAL-DIR
+ (if on-windows?
+ "C:\\storm-local"
+ "/var/storm-local")}]
+ (try
+ (System/setProperty storm-home-key (if on-windows? "C:\\storm-home" "/usr/local/storm-home"))
--- End diff --
@Renkai If test should change storm.home to work correctly, let's remove test.
---
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-1005] set storm.local.dir relative valu...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/722#issuecomment-138382687
@Renkai
Great. Could you add simple unit test to check ```absolute-storm-local-dir``` works with relative / absolute path?
---
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-1005] set storm.local.dir relative valu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/722
---
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-1005] set storm.local.dir relative valu...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/722#issuecomment-138698074
+1
@Renkai I think current unit test is enough.
---
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.
---