You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apex.apache.org by PramodSSImmaneni <gi...@git.apache.org> on 2016/04/22 16:11:19 UTC
[GitHub] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
GitHub user PramodSSImmaneni opened a pull request:
https://github.com/apache/incubator-apex-core/pull/314
APEXCORE-439 Fixing operator redeploy
Just for perusal, need to add unit test.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/PramodSSImmaneni/incubator-apex-core APEXCORE-439
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-apex-core/pull/314.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 #314
----
commit 6cad40e165c29c97c0c11d248058d75d76245649
Author: Pramod Immaneni <pr...@datatorrent.com>
Date: 2016-04-22T14:05:39Z
Setting correct state to redeploy operator, operator will be undeployed first followed by a deploy
----
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/314#issuecomment-213545415
Looking at it. In the final change, please don't do any reformatting related change (imports etc.)
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by amberarrow <gi...@git.apache.org>.
Github user amberarrow commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/314#discussion_r60845111
--- Diff: engine/src/main/java/com/datatorrent/stram/plan/physical/PhysicalPlan.java ---
@@ -979,6 +998,14 @@ public void deployChanges() {
this.undeployOpers.removeAll(newOpers.keySet());
--- End diff --
For consistency, prefix getDependents() with "this." (though I find the unnecessary use of "this" distracting -- my mind immediately starts to scan the code for possible ambiguities when there are none).
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/314#discussion_r60839461
--- Diff: engine/src/main/java/com/datatorrent/stram/plan/physical/PhysicalPlan.java ---
@@ -979,6 +998,14 @@ public void deployChanges() {
this.undeployOpers.removeAll(newOpers.keySet());
--- End diff --
You can add the following before this line (979) and then remove everything added new below:
```
// redeploy dependencies of the new operators excluding the new operators themselves
this.undeployOpers.addAll(getDependents(newOpers.keySet()));
```
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/314#discussion_r60839381
--- Diff: engine/src/main/java/com/datatorrent/stram/plan/physical/PhysicalPlan.java ---
@@ -979,6 +998,14 @@ public void deployChanges() {
this.undeployOpers.removeAll(newOpers.keySet());
//make sure all the new operators are included in deploy operator list
this.deployOpers.addAll(this.newOpers.keySet());
+ // redeploy dependencies of the new operators excluding the new operators themselves
+ Set<PTOperator> ndeps = getDependents(newOpers.keySet());
+ ndeps.removeAll(newOpers.keySet());
+ // don't redeploy operators already going to be undeployed
+ ndeps.removeAll(this.undeployOpers);
+ // redeploy remaining dependencies
+ this.undeployOpers.addAll(ndeps);
+ this.deployOpers.addAll(ndeps);
--- End diff --
This shouldn't be required. Next line already includes dependencies.
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/314#discussion_r60839469
--- Diff: engine/src/test/java/com/datatorrent/stram/plan/physical/PhysicalPlanTest.java ---
@@ -20,25 +20,43 @@
import java.io.Serializable;
-import java.util.*;
--- End diff --
Please remove the import changes. That was already done in master and we want only the net diff for this patch.
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/314#issuecomment-213984857
Yes @amberarrow, there is a test covering that in testMxNPartitioning in PhysicalPlanTest
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/314#discussion_r60846637
--- Diff: engine/src/test/java/com/datatorrent/stram/plan/physical/PhysicalPlanTest.java ---
@@ -20,25 +20,43 @@
import java.io.Serializable;
-import java.util.*;
--- End diff --
@tweise will do, difficult to tell the IDE to let it go
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/314#discussion_r60847753
--- Diff: engine/src/main/java/com/datatorrent/stram/plan/physical/PhysicalPlan.java ---
@@ -979,6 +998,14 @@ public void deployChanges() {
this.undeployOpers.removeAll(newOpers.keySet());
--- End diff --
@amberarrow I see that makes sense
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni closed the pull request at:
https://github.com/apache/incubator-apex-core/pull/314
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by amberarrow <gi...@git.apache.org>.
Github user amberarrow commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/314#issuecomment-213982499
One question: if partitions P{1,2} are shuffled to Q{1,2,3} via unifiers U{1,2.3} and Q is now repartitioned
to Q{1,4} (i.e. Q2 and Q3 are discarded, Q4 is new and Q1 is preserved), will we have these actions:
-- U1 preserved;
-- U{2,3} be killed; and
-- new unifier U4 created and deployed ?
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/314#issuecomment-213896235
@PramodSSImmaneni thanks for tracking this down and adding the test. Please see proposed changes.
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/314#discussion_r60839478
--- Diff: engine/src/main/java/com/datatorrent/stram/plan/physical/PhysicalPlan.java ---
@@ -20,31 +20,50 @@
import java.io.IOException;
import java.io.Serializable;
-import java.util.*;
+import java.util.ArrayList;
--- End diff --
Same as noted below.
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/314#discussion_r60846619
--- Diff: engine/src/main/java/com/datatorrent/stram/plan/physical/PhysicalPlan.java ---
@@ -979,6 +998,14 @@ public void deployChanges() {
this.undeployOpers.removeAll(newOpers.keySet());
--- End diff --
@tweise The getDependents also includes the operators in the argument set, since they are new we don't want to undeploy them. So it should be this
Set<PTOperator> ndeps = getDependents(newOpers.keySet());
ndeps.removeAll(newOpers.keySet());
this.undeployOpers.addAll(ndeps);
As you pointed out ndeps doesn't have to be added to deployOpers as it is already being done in the next line.
I had a line in there
ndeps.removeAll(this.undeployOpers);
to ensure that operators that are already scheduled to be undeployed (removed completely) don't get re-deployed because of this but on further thought this probably is not required because if some operators are already going to be undeployed they will not be in the dependency chain for newly deployable operators, is that assumption valid?
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/314#discussion_r60797952
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingContainerManager.java ---
@@ -2272,7 +2329,7 @@ public void deploy(Set<PTContainer> releaseContainers, Collection<PTOperator> un
for (PTOperator oper : e.getValue()) {
// operator will be deployed after it has been undeployed, if still referenced by the container
if (oper.getState() != PTOperator.State.PENDING_UNDEPLOY) {
- oper.setState(PTOperator.State.PENDING_DEPLOY);
+ oper.setState(PTOperator.State.PENDING_UNDEPLOY);
--- End diff --
Can you please check what happens in the initial for loop that sets all operators to UNDEPLOY? Why isn't the operator marked there?
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by amberarrow <gi...@git.apache.org>.
Github user amberarrow commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/314#discussion_r60847135
--- Diff: engine/src/main/java/com/datatorrent/stram/plan/physical/PhysicalPlan.java ---
@@ -979,6 +998,14 @@ public void deployChanges() {
this.undeployOpers.removeAll(newOpers.keySet());
--- End diff --
If I understand @tweise correctly, he is suggesting that the existing code be augmented _only_ by the highlighted fragment below (since he says "remove everything added new"):
{code}
updatePartitionsInfoForPersistOperator(this.dag);
/* begin new code */
// redeploy dependencies of the new operators excluding the new operators themselves
this.undeployOpers.addAll(getDependents(newOpers.keySet()));
/* end new code */
this.undeployOpers.removeAll(newOpers.keySet());
//make sure all the new operators are included in deploy operator list
this.deployOpers.addAll(this.newOpers.keySet());
// include downstream dependencies of affected operators into redeploy
Set<PTOperator> deployOperators = this.getDependents(this.deployOpers);
ctx.deploy(releaseContainers, this.undeployOpers, newContainers, deployOperators);
{code}
This works for _undeployOpers_ since the argument set (newOpers) is removed just after the new code. However, this change alone does not add dependents to _deployOpers_ as the proposed fix does -- not sure if this makes a difference.
---
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] incubator-apex-core pull request: APEXCORE-439 Fixing operator red...
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/314#discussion_r60846626
--- Diff: engine/src/main/java/com/datatorrent/stram/plan/physical/PhysicalPlan.java ---
@@ -979,6 +998,14 @@ public void deployChanges() {
this.undeployOpers.removeAll(newOpers.keySet());
--- End diff --
@amberarrow will do
---
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.
---