[GitHub] storm issue #1691: STORM-2090: Add integration test for storm windowing
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/1691 @raghavgautam I am +1 overall. I believe it makes sense to have these integration tests run in the mvn integration-test phase, but that's the only detail I have to add. +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 issue #1691: STORM-2090: Add integration test for storm windowing
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1691 @raghavgautam Nope. All is 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1691: STORM-2090: Add integration test for storm windowing
Github user raghavgautam commented on the issue: https://github.com/apache/storm/pull/1691 @ptgoetz Do we need to document any other place or just mentioning on the jira is sufficient ? --- 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 issue #1691: STORM-2090: Add integration test for storm windowing
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1691 Just to be clear, that wasn't a criticism. I just wanted to point out that it is important that we know the provenance and license of all code that enters our repository. --- 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 issue #1691: STORM-2090: Add integration test for storm windowing
Github user raghavgautam commented on the issue: https://github.com/apache/storm/pull/1691 I had mention this on the jira. A good part of the vagrant setup has been picked up from: https://github.com/ptgoetz/storm-vagrant https://github.com/harshach/storm-vagrant https://issues.apache.org/jira/browse/STORM-2090 Both of them have apache license: https://github.com/ptgoetz/storm-vagrant/blob/master/LICENSE https://github.com/harshach/storm-vagrant/blob/master/LICENSE Rest has been authored by me (contributed by Hortonworks). This includes TestngListner.java which was originally contributed to apache falcon. https://github.com/apache/falcon/blob/master/falcon-regression/merlin/src/test/java/org/apache/falcon/regression/TestngListener.java --- 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 issue #1691: STORM-2090: Add integration test for storm windowing
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1691 This a fairly large commit that seemingly includes code from other projects. That's fine as long as you can document what code was copied, and what the license for that code was. --- 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 issue #1691: STORM-2090: Add integration test for storm windowing
Github user raghavgautam commented on the issue: https://github.com/apache/storm/pull/1691 Thanks @harshach @HeartSaVioR for reviewing. --- 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 issue #1691: STORM-2090: Add integration test for storm windowing
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1691 I squashed commits before merging (b779ca4e270f6f2a86d4d30336004e4a642ec690) but forgot to write 'Closes #1691' to commit log. @raghavgautam Could you close this? Thanks! --- 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 issue #1691: STORM-2090: Add integration test for storm windowing
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1691 @raghavgautam Thanks. It looks great to me. +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 issue #1691: STORM-2090: Add integration test for storm windowing
Github user raghavgautam commented on the issue: https://github.com/apache/storm/pull/1691 @HeartSaVioR I have fixed the rat issue and I have put fast fail. Yes, we will be contributing more tests to apache storm going forward. --- 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 issue #1691: STORM-2090: Add integration test for storm windowing
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1691 @raghavgautam 1. Travis CI fails with RAT issue. Could you run your branch via `mvn clean install -Prat`? 2. Could we apply fail-fast? I think integration tests don't need to be run when test is failing. Please refer https://travis-ci.org/apache/storm/jobs/161473780. Test is failing with RAT, and script ran integration tests. Btw, do you plan to introduce more integration tests? Since it seems additional 20 mins to setup and run the new integration tests so I feel it a bit heavy to test just a windowing cases. --- 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. ---
