[GitHub] storm issue #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1786 @revans2 Thanks. Still +1. (One procedural nit: Rebasing/squashing on big PRs like this can make reviews a little harder. E.g. After you added the javadocs I wanted to review just that change, but the squash forced me to wade through all the other changes to find the ones I was looking for. Not a big deal, but something to keep in mind.) --- 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 #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1786 Rebased now that java nimbus is in. --- 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 #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1786 @ptgoetz I added in javadocs for most of the new classes --- 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 #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1786 @ptgoetz I'll look at adding in some javadocs. In general I tried to add javadocs where there were clojure doc strings, but there were not too many of them either. --- 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 #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1786 +1. I haven't done any manual testing of this patch, but I reviewed the code and it looks good. I wouldn't mind seeing more javadoc comments in some of the new classes/interfaces, but it can always be added later, so I don't see that as a merge blocker or something that needs to be addressed right away. --- 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 #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1786 The test failures look unrelated. They seem to be the same windowing integration tests failing 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. ---
