STORM-2090: Add integration test for storm windowing #1691
Conversation
raghavgautam
force-pushed
the
apache_master
branch
from
5d288c5
to
ce1cdd2
Compare
There was a problem hiding this comment.
Great Work @raghavgautam . This will help a lot. Minor nits. +1.
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import javax.annotation.Nullable; | ||
| import java.util.*; |
There was a problem hiding this comment.
can you avoid wildcard imports
| drpc.servers: | ||
| - "node1" | ||
|
|
||
| supervisor.slots.ports: [6700, 6701, 6702, 6703, 6704, 6705, 6706, 6707, 6708, 6709] |
There was a problem hiding this comment.
any reason to have these many worker slots?
There was a problem hiding this comment.
We are running tests in parallel and I have put this for worst case scenario.
Btw, do you plan to introduce more integration tests? |
|
@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. |
|
@raghavgautam Thanks. It looks great to me. +1 |
|
I squashed commits before merging (b779ca4) but forgot to write 'Closes #1691' to commit log. @raghavgautam Could you close this? Thanks! |
|
Thanks @harshach @HeartSaVioR for reviewing. |
|
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. |
|
I had mention this on the jira. https://issues.apache.org/jira/browse/STORM-2090 Both of them have apache license: Rest has been authored by me (contributed by Hortonworks). |
|
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. |
|
@ptgoetz Do we need to document any other place or just mentioning on the jira is sufficient ? |
|
@raghavgautam Nope. All is good. |
| -------------------------- | ||
| To run all tests: | ||
| ```sh | ||
| mvn clean package -DskipTests && mvn test |
There was a problem hiding this comment.
mvn test will make these integration tests run in the maven test phase. I believe it would be more appropriate to have these integration tests run in the maven integration-test or verify phase.
Please refer here for relevant info.
|
@raghavgautam I am +1 overall. I believe it makes sense to have these integration tests run in the mvn integration-test phase. Can you please also squash all the commits into one commit. It will make the git log much cleaner and easy to follow. Besides that i am +1 |
|
|
||
| Running tests end-to-end | ||
| ------------------------ | ||
| Assumption: |
There was a problem hiding this comment.
I would rename Assumption to Requirements, as it is a requirement to have this in order to run the tests
No description provided.