[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/1161 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-191330287 One minor comment, but it is important that we get this and #1172 in, so I am fine with making the change myself as a part of the merge. +1 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/1161#discussion_r54755980 --- Diff: pom.xml --- @@ -914,6 +916,26 @@ org.apache.maven.plugins +maven-antrun-plugin +1.8 + + +install + +run + + + + + + + + + + + --- End diff -- I have had a lot of issues in the past with trying to use antrun to clean. I really would prefer to change it to be something like ``` maven-clean-plugin 2.5 cleanup clean clean true ./logs/ ``` --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user abhishekagarwal87 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-191096651 Thanks @unsleepy22 +1 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-191034221 @abhishekagarwal87 changed according to your comments @revans2 added logs to .gitignore and maven-antrun-plugin to delete logs directory after tests please help review again --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190742680 The changes I see look fine to me I am +1 for merging them in. If we are going to go with this patch I would like to see the logs dir deleted as part `mvn clean` and I would like to see it added to `.gitignore` so we don't add log files by accident. I personally think that a final solution should include changing how we do logging when in local mode. I would disable the dynamic log configuration for workers when in local mode, and I would disable the event logger configuration when in local mode. I have seen errors when I try to run in local mode and the permissions for log files in `storm.home` or `storm.log.dir` are not quite set properly. If I am running in local mode I should not be writing anything to `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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user abhishekagarwal87 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190730160 If you are taking my suggestions into consideration, you also need to remove these from getLogDir() func ``` } else if (System.getProperty("storm.local.dir") != null) { +dir = System.getProperty("storm.local.dir") + FILE_SEPARATOR + "logs"; +} else if (conf.get("storm.local.dir") != null) { +dir = conf.get("storm.local.dir") + FILE_SEPARATOR + "logs"; +} ``` --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190727124 Ah, you're right, I get 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user abhishekagarwal87 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190723316 Thanks @unsleepy22 . Logs directory should not be created from the path where the daemon is started from. If storm.log.dir had resolved to ${storm.home}/logs, it would have been fine. But that is not happening here. Different daemons (worker, logviewer, supervisor) can be started from different paths. That means their logs will go into different locations. Each daemon must see the same path of base log dir. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190713869 @abhishekagarwal87 your change is more careful & considerate, but changing defaults.yaml would be simpler and it's pretty reasonable to set storm.log.dir to "logs" by default, what do you think? Also, @revans2 what's your opinion on 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user abhishekagarwal87 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190696088 In ConfigUtils.getLogDir() replace ``` } else { dir = concatIfNotNull(System.getProperty("storm.home")) + FILE_SEPARATOR + "logs"; ``` with ``` } else if (System.getProperty("storm.home") != null) { dir = System.getProperty("storm.home") + FILE_SEPARATOR + "logs"; } else { dir = "logs"; ``` Revert the changes in defaults.yaml and keep the changes in pom.xml and supervisor test. storm.home is not set and thus log dir is being set to /logs. With the above changes in test, it will be set to logs. So it should work. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190684819 Probably won't work since storm.home is not set when running tests. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user abhishekagarwal87 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190670626 the logs directory resolves to current path (from where the command runs) and not the storm home e.g. I set the `storm.log.dir: logs` in my defaults.yaml and ran it from my home directory. the logs were created inside the home directory. You will have to do something like this in getLogDir() function in ConfigUtils ``` if (new File(dir).isAbsolute()) { return dir; } else { return concatIfNotNull(System.getProperty("storm.home")) + FILE_SEPARATOR + dir); } ``` --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190621208 @abhishekagarwal87 changed according to your comments, please help review again, rat check also passes. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user abhishekagarwal87 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190216876 1.Not necessary though redundant after 2. 2. If it is same as storm.log.dir = `concatIfNotNull(System.getProperty("storm.home")) + FILE_SEPARATOR + "logs"` then I don't see any problem. 3. +1 4. +1 I feel the test errors can still be solved with just 3 and 4.. can you tell me which tests throw NoSuchFileException? I will also try out something. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190194178 @abhishekagarwal87 , I get you. My opinion would be: 1. revert changes to ConfigUtils class 2. set "storm.log.dir: logs" in defaults.yaml 3. add "**/logs/**" to apache-rat-plugin excludes 4. remain supervisor-test the same with my commit because settting "storm.log.dir = logs OR /tmp/logs" will break supervisor-test anyway. 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user abhishekagarwal87 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190120350 /logs is an absolute path. Setting `storm.log.dir: logs` may work if it resolves to {storm.home}/logs in worker and other daemons. However, correct me if I am wrong, this will not remove the test exceptions. Instead of hard-coded "/logs", passing a temporary path such as /tmp/storm-logs as the value of storm.log.dir should remove the exceptions. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190108980 @abhishekagarwal87 meaning that add `storm.log.dir: /logs` to defaults.yaml? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user abhishekagarwal87 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190074820 I suspected that the logs directory is being created inside the project itself and failing rat check. I pulled in your changes to check the same. ```find storm-core/storm-local/logs -type f | wc -l 26 ``` This directory is not excluded from the RAT check and thus build is failing. can you try excluding this directory from RAT check and run the build on your local end (mvn clean install) to see if the build succeeds? Sorry to tell this late but I see a problem with these changes I didn't think of before. Earlier the default log location was under logs directory. With these changes, logs will start going in storm-local directory which is meant for internal use. logs directory becomes redundant since storm.loca.dir always has a default value. In fact this PR will break STORM-1552 which @arunmahadevan should confirm. The exceptions in test classes may be fixed without changing the getLogDir implementation (by passing the `storm.local.dir/logs` as storm.log.dir property value instead of hardcoded logs directory) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1161#issuecomment-190003786 Build failure looks odd, I have all tests pass locally. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
GitHub user unsleepy22 opened a pull request: https://github.com/apache/storm/pull/1161 [Storm-1579] Fix NoSuchFileException when running tests in storm-core The old code takes "storm.local.dir" property as logDir while changes in STORM-1552 used ConfigUtils.getLogDir, which doesn't take this property, causing the ultimate event log directory to be "/logs/" and resulting in NoSuchFileException because permission is denied to create /logs directory. The change of code causes 3 test cases to fail in supervisor_test, so I changed them accordingly to make all tests pass. You can merge this pull request into a Git repository by running: $ git pull https://github.com/unsleepy22/storm STORM-1579 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/1161.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 #1161 commit 034f0cf107403100650d6eb65e7168f62133864a Author: å«ä¹Date: 2016-02-28T14:33:21Z fix STORM-1579, checks storm.local.dir property/conf when getting storm log dir commit 504c11b8ead80e186ff0de83dbdece2337cd1162 Author: å«ä¹ Date: 2016-02-28T14:42:56Z append "/logs" to "storm.local.dir" property when non-null --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user unsleepy22 closed the pull request at: https://github.com/apache/storm/pull/1155 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1155#issuecomment-189629712 My change was quite simple: just adding back the logic to check "storm.local.dir" property/conf, but there're 3 test failures in "org.apache.storm.supervisor-test" (all the others pass without exceptions), I've no idea why, can someone please help take a look? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user abhishekagarwal87 commented on the pull request: https://github.com/apache/storm/pull/1155#issuecomment-189295516 ohk. I see. Once the commit is ready, you can squash the commits. Looks good to me otherwise. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1155#issuecomment-189291719 @abhishekagarwal87 they're in test-reports, when running tests, you'll get messages like "Exception: java.lang.ClassCastException thrown from the UncaughtExceptionHandler in thread "Thread-35-__eventlogger-executor[2 2]", by looking into the detailed test-reports, you'll get NoSuchFileException. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user abhishekagarwal87 commented on the pull request: https://github.com/apache/storm/pull/1155#issuecomment-189284515 I didn't see such error on my system while running the tests though --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
Github user unsleepy22 commented on the pull request: https://github.com/apache/storm/pull/1155#issuecomment-189268264 There're test failures, I'll look into it first. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [Storm-1579] Fix NoSuchFileException when runn...
GitHub user unsleepy22 opened a pull request: https://github.com/apache/storm/pull/1155 [Storm-1579] Fix NoSuchFileException when running tests in storm-core The old code takes "storm.local.dir" property as logDir while changes in STORM-1552 used `ConfigUtils.getLogDir`, which doesn't take this property, causing the ultimate event log directory to be "/logs/" and resulting in NoSuchFileException. Note that the only effective change is in `getLogDir` method, I just formatted the whole class. You can merge this pull request into a Git repository by running: $ git pull https://github.com/unsleepy22/storm STORM-1579 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/1155.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 #1155 commit afd2d525be396c6f430e6a4a13cd1f237496a473 Author: å«ä¹Date: 2016-02-24T13:06:25Z port backtype.storm.stats to java commit 52d3b587f07db7dcf66b774531e2face7247c7b6 Author: å«ä¹ Date: 2016-02-24T13:12:53Z add translated java files commit f61ea0c0196da4f31126f3f96ffb2bf5551a01d2 Author: å«ä¹ Date: 2016-02-25T02:59:42Z move update tuple stat/renderStats methods to corresponding ExecutorStat classes commit 67a5878e5f37ccd317c10ef8dcbd56b9de233997 Author: å«ä¹ Date: 2016-02-25T05:10:07Z Merge https://github.com/apache/storm Conflicts: storm-core/src/clj/org/apache/storm/converter.clj storm-core/src/clj/org/apache/storm/daemon/executor.clj storm-core/src/clj/org/apache/storm/daemon/nimbus.clj storm-core/src/clj/org/apache/storm/stats.clj storm-core/src/clj/org/apache/storm/ui/core.clj storm-core/test/clj/org/apache/storm/nimbus_test.clj commit 880134881566427e886b01d44890d22db483f6bd Author: å«ä¹ Date: 2016-02-25T05:11:50Z merge conflicts from master commit e5564c0f888e40af2726a645d24cfad0aaeed26a Author: å«ä¹ Date: 2016-02-25T07:06:59Z added last-error to stats commit 3fc80c4b0bfc83d2534fab160c72894af044dbc3 Author: å«ä¹ Date: 2016-02-25T07:25:58Z fixed a potential NPE commit f819b4999eac052840ff69552198b184afa0c1e0 Author: å«ä¹ Date: 2016-02-26T06:02:45Z merge from master commit e0e9de7d01bb09e6593093cc9324b09f03abb55c Author: å«ä¹ Date: 2016-02-26T06:03:35Z merge from master commit db7fe5730953f5d2dc3bb376303d247c1cd1393b Author: å«ä¹ Date: 2016-02-26T06:06:48Z fix the bug that ClassCastException were thrown when running storm-core tests commit b44233509ef33837a8f4f83882a5b55988336a49 Author: å«ä¹ Date: 2016-02-26T11:17:23Z restore storm-master branch commit d36cf75b71db5da5fd8f1ad8357075ceafaf33aa Author: å«ä¹ Date: 2016-02-26T11:24:29Z Merge https://github.com/apache/storm into storm-master commit 704e5c0179dd12ce29b97998cfd36951fa6bc70e Author: å«ä¹ Date: 2016-02-26T11:32:02Z add stats.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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---