You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by revans2 <gi...@git.apache.org> on 2018/11/15 20:15:17 UTC
[GitHub] storm pull request #2908: STORM-3276: Updated Flux to deal with storm local ...
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/2908
STORM-3276: Updated Flux to deal with storm local correctly
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/revans2/incubator-storm STORM-3276
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2908.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2908
----
commit 150160f534145d568c0368ec19823813ad16136c
Author: Robert (Bobby) Evans <ev...@...>
Date: 2018-11-15T20:12:48Z
STORM-3276: Updated Flux to deal with storm local correctly
----
---
[GitHub] storm issue #2908: STORM-3276: Updated Flux to deal with storm local correct...
Posted by jnioche <gi...@git.apache.org>.
Github user jnioche commented on the issue:
https://github.com/apache/storm/pull/2908
Tested on a StormCrawler topology, works fine in local and remote mode.
one quick question though: shouldn't --sleep be deprecated? --local-ttl is used for all the local jobs instead
---
[GitHub] storm pull request #2908: STORM-3276: Updated Flux to deal with storm local ...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2908#discussion_r236358832
--- Diff: flux/flux-core/src/main/java/org/apache/storm/flux/Flux.java ---
@@ -52,17 +52,22 @@
public class Flux {
private static final Logger LOG = LoggerFactory.getLogger(Flux.class);
+ @Deprecated
--- End diff --
Given that current scripts to start a Flux topology will have to be changed to make this work, is there any value to preserving these options? If someone is using one of these, most likely their script will end up failing or doing the wrong thing.
---
[GitHub] storm pull request #2908: STORM-3276: Updated Flux to deal with storm local ...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2908#discussion_r236360412
--- Diff: storm-server/src/main/java/org/apache/storm/LocalCluster.java ---
@@ -323,6 +342,13 @@ private static boolean areAllWorkersWaiting() {
}
}
+ private static final long DEFAULT_ZK_PORT = 2181;
--- End diff --
Nit: Please move this to the other constants so it's not in the middle of method declarations.
---
[GitHub] storm pull request #2908: STORM-3276: Updated Flux to deal with storm local ...
Posted by jnioche <gi...@git.apache.org>.
Github user jnioche commented on a diff in the pull request:
https://github.com/apache/storm/pull/2908#discussion_r237006771
--- Diff: flux/flux-core/src/main/java/org/apache/storm/flux/Flux.java ---
@@ -52,17 +52,22 @@
public class Flux {
private static final Logger LOG = LoggerFactory.getLogger(Flux.class);
+ @Deprecated
--- End diff --
Got you! Your comment was pretty clear, I just need another coffee :-)
---
[GitHub] storm pull request #2908: STORM-3276: Updated Flux to deal with storm local ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2908#discussion_r234303902
--- Diff: storm-server/src/main/java/org/apache/storm/LocalCluster.java ---
@@ -1197,8 +1238,9 @@ public IBolt makeAckerBoltImpl() {
* When running a topology locally, for tests etc. It is helpful to be sure that the topology is dead before the test exits. This is
* an AutoCloseable topology that not only gives you access to the compiled StormTopology but also will kill the topology when it
* closes.
- *
+ * ```
--- End diff --
I generated the javadocs and the markdown plugin didn't work in this case. I am not sure why, so I cleaned it up.
---
[GitHub] storm pull request #2908: STORM-3276: Updated Flux to deal with storm local ...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2908#discussion_r234019467
--- Diff: storm-server/src/main/java/org/apache/storm/LocalCluster.java ---
@@ -1197,8 +1238,9 @@ public IBolt makeAckerBoltImpl() {
* When running a topology locally, for tests etc. It is helpful to be sure that the topology is dead before the test exits. This is
* an AutoCloseable topology that not only gives you access to the compiled StormTopology but also will kill the topology when it
* closes.
- *
+ * ```
--- End diff --
Minor one. Not sure if ``` turns it into pre formatted code. Do we need to use `<code`> tag?
---
[GitHub] storm issue #2908: STORM-3276: Updated Flux to deal with storm local correct...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2908
@srdo @kishorvpatil @jnioche
Sorry this has taken so long. I just rebased/squashed and @srdo I addressed your nit. Please take another look.
---
[GitHub] storm pull request #2908: STORM-3276: Updated Flux to deal with storm local ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2908
---
[GitHub] storm pull request #2908: STORM-3276: Updated Flux to deal with storm local ...
Posted by jnioche <gi...@git.apache.org>.
Github user jnioche commented on a diff in the pull request:
https://github.com/apache/storm/pull/2908#discussion_r237004334
--- Diff: flux/flux-core/src/main/java/org/apache/storm/flux/Flux.java ---
@@ -52,17 +52,22 @@
public class Flux {
private static final Logger LOG = LoggerFactory.getLogger(Flux.class);
+ @Deprecated
--- End diff --
IMHO this it is acceptable for a major release. Projects need a drastic cleanup once in a while.
---
[GitHub] storm issue #2908: STORM-3276: Updated Flux to deal with storm local correct...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2908
@jnioche you are 100% correct. I'll deprecate sleep too.
---
[GitHub] storm pull request #2908: STORM-3276: Updated Flux to deal with storm local ...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2908#discussion_r237005496
--- Diff: flux/flux-core/src/main/java/org/apache/storm/flux/Flux.java ---
@@ -52,17 +52,22 @@
public class Flux {
private static final Logger LOG = LoggerFactory.getLogger(Flux.class);
+ @Deprecated
--- End diff --
Sure, what I meant was if we're going to break the API anyway I think it makes sense to remove the options entirely, rather than deprecating them.
---
[GitHub] storm issue #2908: STORM-3276: Updated Flux to deal with storm local correct...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2908
+1.
I'm still wondering why we need to preserve command line options here https://github.com/apache/storm/pull/2908#discussion_r236358832 but I think it's fine either way.
---