You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apex.apache.org by davidyan74 <gi...@git.apache.org> on 2016/02/19 01:50:04 UTC
[GitHub] incubator-apex-core pull request: APEXCORE-201
GitHub user davidyan74 opened a pull request:
https://github.com/apache/incubator-apex-core/pull/230
APEXCORE-201
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/davidyan74/incubator-apex-core APEXCORE-201-PR
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-apex-core/pull/230.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 #230
----
commit d67d7711fa16b76d068e4f1e9c2d46d8668fc7a4
Author: David Yan <da...@datatorrent.com>
Date: 2015-12-30T01:35:04Z
APEXCORE-201 changed the way latency is calculated and fixed the problem when latency is stalled when an operator falls behind too many windows
commit dcb47f22b7f9e3a0462487f55322eb42b550ff2d
Author: David Yan <da...@datatorrent.com>
Date: 2016-02-19T00:43:17Z
APEXCORE-201 optimization of finding critical path
----
---
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-201
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-apex-core/pull/230
---
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-201
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/230#discussion_r54947778
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingContainerManager.java ---
@@ -230,7 +231,16 @@ protected boolean removeEldestEntry(Map.Entry<String, ContainerInfo> eldest)
public static class CriticalPathInfo
--- End diff --
Can this class be declared as private?
---
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-201
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/230#discussion_r54947946
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingContainerManager.java ---
@@ -230,7 +231,16 @@ protected boolean removeEldestEntry(Map.Entry<String, ContainerInfo> eldest)
public static class CriticalPathInfo
{
long latency;
- LinkedList<Integer> path = new LinkedList<Integer>();
+ LinkedList<Integer> path = new LinkedList<>();
--- End diff --
Introduce private copy constructor to avoid creating empty LinkedList?
---
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-201
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/230#discussion_r55256158
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingContainerManager.java ---
@@ -919,106 +914,41 @@ private void saveMetaInfo() throws IOException
return logicalMetrics.get(operatorName);
}
- private void calculateLatency(PTOperator oper, Map<Integer, EndWindowStats> endWindowStatsMap, Set<PTOperator> endWindowStatsVisited, Set<PTOperator> leafOperators)
+ private CriticalPathInfo findCriticalPath()
{
- endWindowStatsVisited.add(oper);
- OperatorStatus operatorStatus = oper.stats;
-
- EndWindowStats endWindowStats = endWindowStatsMap.get(oper.getId());
- if (endWindowStats == null) {
- LOG.info("End window stats is null for operator {}, probably a new operator after partitioning", oper);
- return;
- }
-
- // find the maximum end window emit time from all input ports
- long upstreamMaxEmitTimestamp = -1;
- PTOperator upstreamMaxEmitTimestampOperator = null;
- for (PTOperator.PTInput input : oper.getInputs()) {
- if (null != input.source.source) {
- PTOperator upstreamOp = input.source.source;
- EndWindowStats upstreamEndWindowStats = endWindowStatsMap.get(upstreamOp.getId());
- if (upstreamEndWindowStats == null) {
- LOG.info("End window stats is null for operator {}", oper);
- return;
- }
- long adjustedEndWindowEmitTimestamp = upstreamEndWindowStats.emitTimestamp;
- MovingAverageLong rpcLatency = rpcLatencies.get(upstreamOp.getContainer().getExternalId());
- if (rpcLatency != null) {
- adjustedEndWindowEmitTimestamp += rpcLatency.getAvg();
- }
- if (adjustedEndWindowEmitTimestamp > upstreamMaxEmitTimestamp) {
- upstreamMaxEmitTimestamp = adjustedEndWindowEmitTimestamp;
- upstreamMaxEmitTimestampOperator = upstreamOp;
- }
- }
- }
-
- if (upstreamMaxEmitTimestamp > 0) {
- long adjustedEndWindowEmitTimestamp = endWindowStats.emitTimestamp;
- MovingAverageLong rpcLatency = rpcLatencies.get(oper.getContainer().getExternalId());
- if (rpcLatency != null) {
- adjustedEndWindowEmitTimestamp += rpcLatency.getAvg();
- }
- if (upstreamMaxEmitTimestamp <= adjustedEndWindowEmitTimestamp) {
- LOG.debug("Adding {} to latency MA for {}", adjustedEndWindowEmitTimestamp - upstreamMaxEmitTimestamp, oper);
- operatorStatus.latencyMA.add(adjustedEndWindowEmitTimestamp - upstreamMaxEmitTimestamp);
- } else {
- operatorStatus.latencyMA.add(0);
- if (lastLatencyWarningTime < System.currentTimeMillis() - LATENCY_WARNING_THRESHOLD_MILLIS) {
- LOG.warn("Latency calculation for this operator may not be correct because upstream end window timestamp is greater than this operator's end window timestamp: {} ({}) > {} ({}). Please verify that the system clocks are in sync in your cluster. You can also try tweaking the RPC_LATENCY_COMPENSATION_SAMPLES application attribute (currently set to {}).",
- upstreamMaxEmitTimestamp, upstreamMaxEmitTimestampOperator, adjustedEndWindowEmitTimestamp, oper, this.vars.rpcLatencyCompensationSamples);
- lastLatencyWarningTime = System.currentTimeMillis();
- }
- }
- }
-
- if (oper.getOutputs().isEmpty()) {
- // it is a leaf operator
- leafOperators.add(oper);
- }
- else {
- for (PTOperator.PTOutput output : oper.getOutputs()) {
- for (PTOperator.PTInput input : output.sinks) {
- if (input.target != null) {
- PTOperator downStreamOp = input.target;
- if (!endWindowStatsVisited.contains(downStreamOp)) {
- calculateLatency(downStreamOp, endWindowStatsMap, endWindowStatsVisited, leafOperators);
- }
- }
- }
+ CriticalPathInfo result = null;
+ List<PTOperator> leafOperators = plan.getLeafOperators();
+ Map<PTOperator, CriticalPathInfo> cache = new HashMap<>();
+ for (PTOperator leafOperator : leafOperators) {
+ CriticalPathInfo cpi = findCriticalPathHelper(leafOperator, cache);
+ if (result == null || result.latency < cpi.latency) {
+ result = cpi;
}
}
+ return result;
}
- /*
- * returns cumulative latency
- */
- private long findCriticalPath(Map<Integer, EndWindowStats> endWindowStatsMap, Set<PTOperator> operators, LinkedList<Integer> criticalPath)
- {
- long maxEndWindowTimestamp = 0;
- PTOperator maxOperator = null;
- for (PTOperator operator : operators) {
- EndWindowStats endWindowStats = endWindowStatsMap.get(operator.getId());
- if (maxEndWindowTimestamp < endWindowStats.emitTimestamp) {
- maxEndWindowTimestamp = endWindowStats.emitTimestamp;
- maxOperator = operator;
- }
- }
- if (maxOperator == null) {
- return 0;
- }
- criticalPath.addFirst(maxOperator.getId());
- OperatorStatus operatorStatus = maxOperator.stats;
- operators.clear();
- if (maxOperator.getInputs() == null || maxOperator.getInputs().isEmpty()) {
- return operatorStatus.latencyMA.getAvg();
+ private CriticalPathInfo findCriticalPathHelper(PTOperator operator, Map<PTOperator, CriticalPathInfo> cache)
+ {
+ if (cache.containsKey(operator)) {
--- End diff --
@vrozov done
---
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-201
Posted by PramodSSImmaneni <gi...@git.apache.org>.
Github user PramodSSImmaneni commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/230#issuecomment-193402196
If an operator gets stuck can we improve the accuracy of the result by using a delay estimate for it?
---
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-201
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/230#issuecomment-195142304
@PramodSSImmaneni, the stuck operator scenario would require changes to a different part of the code. The change in this PR is only along the processing path of end window stats. We can address it separately.
---
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-201
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/230#discussion_r54943874
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingContainerManager.java ---
@@ -919,106 +914,41 @@ private void saveMetaInfo() throws IOException
return logicalMetrics.get(operatorName);
}
- private void calculateLatency(PTOperator oper, Map<Integer, EndWindowStats> endWindowStatsMap, Set<PTOperator> endWindowStatsVisited, Set<PTOperator> leafOperators)
+ private CriticalPathInfo findCriticalPath()
{
- endWindowStatsVisited.add(oper);
- OperatorStatus operatorStatus = oper.stats;
-
- EndWindowStats endWindowStats = endWindowStatsMap.get(oper.getId());
- if (endWindowStats == null) {
- LOG.info("End window stats is null for operator {}, probably a new operator after partitioning", oper);
- return;
- }
-
- // find the maximum end window emit time from all input ports
- long upstreamMaxEmitTimestamp = -1;
- PTOperator upstreamMaxEmitTimestampOperator = null;
- for (PTOperator.PTInput input : oper.getInputs()) {
- if (null != input.source.source) {
- PTOperator upstreamOp = input.source.source;
- EndWindowStats upstreamEndWindowStats = endWindowStatsMap.get(upstreamOp.getId());
- if (upstreamEndWindowStats == null) {
- LOG.info("End window stats is null for operator {}", oper);
- return;
- }
- long adjustedEndWindowEmitTimestamp = upstreamEndWindowStats.emitTimestamp;
- MovingAverageLong rpcLatency = rpcLatencies.get(upstreamOp.getContainer().getExternalId());
- if (rpcLatency != null) {
- adjustedEndWindowEmitTimestamp += rpcLatency.getAvg();
- }
- if (adjustedEndWindowEmitTimestamp > upstreamMaxEmitTimestamp) {
- upstreamMaxEmitTimestamp = adjustedEndWindowEmitTimestamp;
- upstreamMaxEmitTimestampOperator = upstreamOp;
- }
- }
- }
-
- if (upstreamMaxEmitTimestamp > 0) {
- long adjustedEndWindowEmitTimestamp = endWindowStats.emitTimestamp;
- MovingAverageLong rpcLatency = rpcLatencies.get(oper.getContainer().getExternalId());
- if (rpcLatency != null) {
- adjustedEndWindowEmitTimestamp += rpcLatency.getAvg();
- }
- if (upstreamMaxEmitTimestamp <= adjustedEndWindowEmitTimestamp) {
- LOG.debug("Adding {} to latency MA for {}", adjustedEndWindowEmitTimestamp - upstreamMaxEmitTimestamp, oper);
- operatorStatus.latencyMA.add(adjustedEndWindowEmitTimestamp - upstreamMaxEmitTimestamp);
- } else {
- operatorStatus.latencyMA.add(0);
- if (lastLatencyWarningTime < System.currentTimeMillis() - LATENCY_WARNING_THRESHOLD_MILLIS) {
- LOG.warn("Latency calculation for this operator may not be correct because upstream end window timestamp is greater than this operator's end window timestamp: {} ({}) > {} ({}). Please verify that the system clocks are in sync in your cluster. You can also try tweaking the RPC_LATENCY_COMPENSATION_SAMPLES application attribute (currently set to {}).",
- upstreamMaxEmitTimestamp, upstreamMaxEmitTimestampOperator, adjustedEndWindowEmitTimestamp, oper, this.vars.rpcLatencyCompensationSamples);
- lastLatencyWarningTime = System.currentTimeMillis();
- }
- }
- }
-
- if (oper.getOutputs().isEmpty()) {
- // it is a leaf operator
- leafOperators.add(oper);
- }
- else {
- for (PTOperator.PTOutput output : oper.getOutputs()) {
- for (PTOperator.PTInput input : output.sinks) {
- if (input.target != null) {
- PTOperator downStreamOp = input.target;
- if (!endWindowStatsVisited.contains(downStreamOp)) {
- calculateLatency(downStreamOp, endWindowStatsMap, endWindowStatsVisited, leafOperators);
- }
- }
- }
+ CriticalPathInfo result = null;
+ List<PTOperator> leafOperators = plan.getLeafOperators();
+ Map<PTOperator, CriticalPathInfo> cache = new HashMap<>();
+ for (PTOperator leafOperator : leafOperators) {
+ CriticalPathInfo cpi = findCriticalPathHelper(leafOperator, cache);
+ if (result == null || result.latency < cpi.latency) {
+ result = cpi;
}
}
+ return result;
}
- /*
- * returns cumulative latency
- */
- private long findCriticalPath(Map<Integer, EndWindowStats> endWindowStatsMap, Set<PTOperator> operators, LinkedList<Integer> criticalPath)
- {
- long maxEndWindowTimestamp = 0;
- PTOperator maxOperator = null;
- for (PTOperator operator : operators) {
- EndWindowStats endWindowStats = endWindowStatsMap.get(operator.getId());
- if (maxEndWindowTimestamp < endWindowStats.emitTimestamp) {
- maxEndWindowTimestamp = endWindowStats.emitTimestamp;
- maxOperator = operator;
- }
- }
- if (maxOperator == null) {
- return 0;
- }
- criticalPath.addFirst(maxOperator.getId());
- OperatorStatus operatorStatus = maxOperator.stats;
- operators.clear();
- if (maxOperator.getInputs() == null || maxOperator.getInputs().isEmpty()) {
- return operatorStatus.latencyMA.getAvg();
+ private CriticalPathInfo findCriticalPathHelper(PTOperator operator, Map<PTOperator, CriticalPathInfo> cache)
+ {
+ if (cache.containsKey(operator)) {
--- End diff --
Avoid double hash map lookup? Combine containsKey() and get()?
---
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-201
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/230#discussion_r55257973
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingContainerManager.java ---
@@ -230,7 +231,16 @@ protected boolean removeEldestEntry(Map.Entry<String, ContainerInfo> eldest)
public static class CriticalPathInfo
{
long latency;
- LinkedList<Integer> path = new LinkedList<Integer>();
+ LinkedList<Integer> path = new LinkedList<>();
--- End diff --
@vrozov done
---
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-201
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/230#discussion_r55252412
--- Diff: engine/src/main/java/com/datatorrent/stram/StreamingContainerManager.java ---
@@ -230,7 +231,16 @@ protected boolean removeEldestEntry(Map.Entry<String, ContainerInfo> eldest)
public static class CriticalPathInfo
--- End diff --
@vrozov The class is actually exposed outside of StreamingContainerManager via a get method.
---
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-201
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/230#issuecomment-195144886
@PramodSSImmaneni I opened a separate JIRA to track this issue: https://issues.apache.org/jira/browse/APEXCORE-379
---
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-201
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/230#issuecomment-185999278
This PR is different from #194:
- critical path calculation no longer visits the same nodes twice
- critical path calculation now takes care of delay loops
@tweise @PramodSSImmaneni please review
@gauravgopi123 I implemented the optimization you mentioned last time
---
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-201
Posted by davidyan74 <gi...@git.apache.org>.
Github user davidyan74 commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/230#issuecomment-195143173
@vrozov squashed. 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 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-201
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/230#issuecomment-195109499
The rest looks good to me. Please squash commits.
---
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.
---