Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Merged via df8346ce1a61ccad2a0c3f715b6c3a53bdaf5670
---
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
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Changed module name as suggestion from pull request on master branch.
---
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
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Rebased with current 1.x-branch 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
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Rebased with current 1.x-branch.
---
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
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Done. Fixed things after receiving +1 are here:
1. nimbus.clj: just log exception instead of crashing nimbus when
blob-rm-dependency-jars-in-topology
2.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
I found a bug while testing other stuff with applied cluster.
StormSubmitter doesn't handle empty --jars option properly. Will address
this shortly both this and PR for master branch.
---
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@harshach @satishd @manuzhang
I just fixed one thing from nimbus.clj: just log exception instead of
crashing nimbus when `blob-rm-dependency-jars-in-topology` throws Exception.
Before that
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
OK, I created pull request against master: #1626
---
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
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1608
+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
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Please hold off merging. I'll create pull request against master. We can
merge both of them together.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user manuzhang commented on the issue:
https://github.com/apache/storm/pull/1608
The exception is caused by the escaping "\" character previously. Things
are working now after I updated to the latest version.
---
If your project is set up for it, you can reply to this email
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@manuzhang A bit confusing so would like to double check on this. Do you
succeed to resolve 1.1.1-SNAPSHOT after `mvn clean install`? Or you're still
failing?
---
If your project is set up for
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@manuzhang OK great. Thanks for the update.
---
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
Github user manuzhang commented on the issue:
https://github.com/apache/storm/pull/1608
@HeartSaVioR yes, I did so and verified I have all the dependencies
locally. I also modified the `DependencyResolverTest` to resolve
`1.1.1-SNAPSHOT` and the UT passed.
---
If your project is
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@manuzhang 1.1.1-SNAPSHOT is not officially released (we don't release
SNAPSHOT officially) so you need to build entire Storm code with `mvn clean
install` before doing that. We can change the
Github user manuzhang commented on the issue:
https://github.com/apache/storm/pull/1608
@HeartSaVioR The code looks good (non-binding). I tried running `storm sql`
but ended up with timeout exceptions
```
Exception in thread "main" java.lang.RuntimeException:
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@harshach Sure, I'll wait for the review during this week. Btw, Actual
modification is around (or less than) 1000 lines, not that much.
---
If your project is set up for it, you can reply to
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1608
@HeartSaVioR can you give it 2 days given that its a big patch. 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
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@satishd
Thanks for the thoughtful review and vote. I rebased the commits into one.
@revans2 @manuzhang @abhishekagarwal87
Please continue reviewing. I'll merge this by tomorrow
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/1608
+1
@HeartSaVioR Nice 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
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@manuzhang Thanks, I addressed your latest comments from
https://github.com/apache/storm/pull/1608/commits/a9f4886fd2f093c7a4450b55ef809fc9826c820e
Not sure we have better representation
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/1608
@HeartSaVioR Overall LGTM. I will revisit once the comments are
addressed/resolved.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@revans2 @satishd @abhishekagarwal87
Please review again when you get some time. I'll squash the commits and go
on porting this to master branch once some of us give +1 for this.
Thanks
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@manuzhang Thanks for checking the details of documentation.
Please leave a comment when you're done reviewing of this. Thanks!
---
If your project is set up for it, you can reply to this
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Porting this to master forces me to resolve huge conflict or modify the
code manually. We shouldn't leave progress of Java port as it is.
---
If your project is set up for it, you can reply to
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Added documentation, and replace packages to artifacts since it's clearer
for its meaning.
I think it's done with 1.x branch, so I'll go on porting this to master
branch.
---
If your
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@revans2 @satishd
Addressed removing jars when topology is killed. Please note that artifacts
will be not removed since it's shared among whole topologies.
Since it modifies Nimbus, we
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@abhishekagarwal87
Thanks, I took a look and your modification is simpler.
Btw, I was thinking same approach, but there're some cases to handle while
assembling jars (for example, the
Github user abhishekagarwal87 commented on the issue:
https://github.com/apache/storm/pull/1608
@HeartSaVioR - not sure if it will be of help, but you can also checkout
https://github.com/apache/storm/pull/1296/files. I was trying out something
similar.
---
If your project is set
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
@revans2 @satishd
Yes I've excluded that for simplification. Since this is a new feature I'd
love to show that this concept works.
Moreover, in order to do that Nimbus should be modified
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/1608
@revans2 This issue was raised in the proposal discussion
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/1608
Conceptually I like the idea, and overall the code looks good. My biggest
concern is around cleanup of the bobs after the topology finishes. I don't see
anywhere in the code that it is doing that,
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
[STORM-2016](http://issues.apache.org/jira/browse/STORM-2016) can also
resolve [STORM-1435](http://issues.apache.org/jira/browse/STORM-1435) via
applying different strategy, specifying
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Builds for PR against 1.x branch is failing and I fixed that from #1609.
I'll rebase once #1609 is merged to 1.x-branch.
---
If your project is set up for it, you can reply to this email and
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1608
Fixed build failure on supervisor_test.
---
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
35 matches
Mail list logo
