[GitHub] storm issue #1744: STORM-1276: line for line translation of nimbus to java
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1744 I am merging I am still happy to make changes on follow on JIRA. --- 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 #1744: STORM-1276: line for line translation of nimbus to java
Github user srdo commented on the issue: https://github.com/apache/storm/pull/1744 I only really have nitpicks, so I'm good with merging too. Really nice job. --- 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 #1744: STORM-1276: line for line translation of nimbus to java
Github user erikdw commented on the issue: https://github.com/apache/storm/pull/1744 @revans2 : Since I didn't have any substantive blocking commits I'm ok with merging it. And it's been long enough that I think you're probably ok to merge away. Any hidden problems will hopefully be surfaced by subsequent testing by others when this is in the mainline. --- 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 #1744: STORM-1276: line for line translation of nimbus to java
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1744 @erikdw I addressed your nits. I appreciate the nits because we don't have a coding standard yet. If we are just down to nits I really would like to get this 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 #1744: STORM-1276: line for line translation of nimbus to java
Github user abellina commented on the issue: https://github.com/apache/storm/pull/1744 nimbus.clj, I made it to mkAssignments. Will continue from there later: - nit: makeBlobCachMap -> makeBlobCacheMap - change: `numDesiredWorkers < numAssignedWorkers` to `numDesiredWorkers > numAssignedWorkers` in: https://github.com/revans2/incubator-storm/blob/39148edf07f2028c2edcfecc53ea9bfac525988f/storm-core/src/jvm/org/apache/storm/daemon/nimbus/Nimbus.java#L1605 - in getResourcesForTopology: do we care if the values are set? (is_set_cpu for example). Those values are doubles. - nit: update comment //TODO remove both of swaps below at first opportunity. But yeah "uglyResources" is right :) --- 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 #1744: STORM-1276: line for line translation of nimbus to java
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1744 @ptgoetz I totally agree on refactoring it. I did a little as I ported it over, but nothing like it needs. I also have done some manual testing. --- 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 #1744: STORM-1276: line for line translation of nimbus to java
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1744 Nice (and a lot of ;) ) work! Nimbus.java is a little unwieldy, but I understand why. Maybe it can be a target for some refactoring later on. +1 (Note that I haven't manually tested yet.) --- 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 #1744: STORM-1276: line for line translation of nimbus to java
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1744 The test failures look unrelated. It would really be nice if someone could fix the TumblingWindowTest and SlidingWindowTest. They fail way too often. --- 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 #1744: STORM-1276: line for line translation of nimbus to java
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1744 I just rebased on top of the latest master (with a java worker translation) --- 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 #1744: STORM-1276: line for line translation of nimbus to java
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1744 Just fixed the merge conflict, and reran the 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 issue #1744: STORM-1276: line for line translation of nimbus to java
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1744 @dossett if you want to build the branch and do some manual testing with your own topologies especially looking at the UI that would be helpful. I did it, but only with word count, and I looked at the UI, but I don't know how exhaustive it was. So any help you can give would be great. --- 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 #1744: STORM-1276: line for line translation of nimbus to java
Github user dossett commented on the issue: https://github.com/apache/storm/pull/1744 I don't know enough about nimbus or clojure to vote on this PR, but this is an exciting change! --- 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. ---
