You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by HeartSaVioR <gi...@git.apache.org> on 2016/08/08 09:07:38 UTC
[GitHub] storm pull request #1616: STORM-2026 Inconsistency between (SpoutExecutor, B...
GitHub user HeartSaVioR opened a pull request:
https://github.com/apache/storm/pull/1616
STORM-2026 Inconsistency between (SpoutExecutor, BoltExecutor) and (spout-transfer-fn, bolt-transfer-fn)
* fix Executor, SpoutExecutor, BoltExecutor to not calling init() while creating Executor itself
* call init() for the first time in async loop
Please refer https://github.com/apache/storm/pull/1445#discussion_r72933087 and comment thread for this.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HeartSaVioR/storm STORM-2026
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1616.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 #1616
----
commit aa8e94c43dab44d1ef8f85adb0f7662894efb2f0
Author: Jungtaek Lim <ka...@gmail.com>
Date: 2016-08-08T09:02:07Z
STORM-2026 Inconsistency between (SpoutExecutor, BoltExecutor) and (spout-transfer-fn, bolt-transfer-fn)
* fix Executor, SpoutExecutor, BoltExecutor to not calling init() while creating Executor itself
* call init() for the first time in async loop
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1616: STORM-2026 Inconsistency between (SpoutExecutor, BoltExec...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/1616
+1, LGTM
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1616: STORM-2026 Inconsistency between (SpoutExecutor, B...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1616#discussion_r73983706
--- Diff: storm-core/src/jvm/org/apache/storm/executor/Executor.java ---
@@ -204,8 +204,8 @@ public static Executor mkExecutor(Map workerData, List<Long> executorId, Map<Str
throw Utils.wrapInRuntime(ex);
}
}
- executor.init(idToTask);
+ executor.idToTask = idToTask;
--- End diff --
Yes, actually the code style is more likely to clojure itself indeed. It's needed for ease of port work, and we can address later.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1616: STORM-2026 Inconsistency between (SpoutExecutor, B...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1616#discussion_r73856052
--- Diff: storm-core/src/jvm/org/apache/storm/executor/Executor.java ---
@@ -204,8 +204,8 @@ public static Executor mkExecutor(Map workerData, List<Long> executorId, Map<Str
throw Utils.wrapInRuntime(ex);
}
}
- executor.init(idToTask);
+ executor.idToTask = idToTask;
--- End diff --
@HeartSaVioR It is better to pass `idToTask` object as part of constructor instead of setting here. idToTask can be created before calling constructor.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1616: STORM-2026 Inconsistency between (SpoutExecutor, B...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1616#discussion_r73975492
--- Diff: storm-core/src/jvm/org/apache/storm/executor/Executor.java ---
@@ -204,8 +204,8 @@ public static Executor mkExecutor(Map workerData, List<Long> executorId, Map<Str
throw Utils.wrapInRuntime(ex);
}
}
- executor.init(idToTask);
+ executor.idToTask = idToTask;
--- End diff --
@satishd
Constructor of Task needs executor so Executor should be created prior to construct idToTask.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1616: STORM-2026 Inconsistency between (SpoutExecutor, BoltExec...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1616
Thanks @satishd for reviewing and merging!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1616: STORM-2026 Inconsistency between (SpoutExecutor, B...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1616
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1616: STORM-2026 Inconsistency between (SpoutExecutor, B...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1616#discussion_r73873775
--- Diff: storm-core/src/jvm/org/apache/storm/executor/spout/SpoutExecutor.java ---
@@ -79,8 +79,11 @@ public SpoutExecutor(final Map workerData, final List<Long> executorId, Map<Stri
this.spoutThrottlingMetrics = new SpoutThrottlingMetrics();
}
- @Override
public void init(final Map<Integer, Task> idToTask) {
+ while (!stormActive.get()) {
--- End diff --
The assumption here is that this method is invoked only once. Do no we want to handle for multiple invocations?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1616: STORM-2026 Inconsistency between (SpoutExecutor, B...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1616#discussion_r73982987
--- Diff: storm-core/src/jvm/org/apache/storm/executor/Executor.java ---
@@ -204,8 +204,8 @@ public static Executor mkExecutor(Map workerData, List<Long> executorId, Map<Str
throw Utils.wrapInRuntime(ex);
}
}
- executor.init(idToTask);
+ executor.idToTask = idToTask;
--- End diff --
@HeartSaVioR Sorry, missed that. This code looks convoluted because it is ported from clojure. I am fine with that for now as these PRs are more about poring from clojure to Java. We can address these later.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1616: STORM-2026 Inconsistency between (SpoutExecutor, B...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1616#discussion_r73975193
--- Diff: storm-core/src/jvm/org/apache/storm/executor/spout/SpoutExecutor.java ---
@@ -79,8 +79,11 @@ public SpoutExecutor(final Map workerData, final List<Long> executorId, Map<Stri
this.spoutThrottlingMetrics = new SpoutThrottlingMetrics();
}
- @Override
public void init(final Map<Integer, Task> idToTask) {
+ while (!stormActive.get()) {
--- End diff --
I just follow previous executor implementation, and invoking init() to multiple times is not intended not only for while statement, but also rest of statements.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1616: STORM-2026 Inconsistency between (SpoutExecutor, BoltExec...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/1616
Thanks @HeartSaVioR
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---