[
https://issues.apache.org/jira/browse/TEZ-2410?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14533146#comment-14533146
]
Bikas Saha commented on TEZ-2410:
---------------------------------
Shouldnt vertexGroup.isCommitted=true be replaced by
vertexGroup.commitStarted=true ? This is when the commit process starts, right?
Without this vertexGroup.isInCommitting() will return false. Not sure how tests
are passing with this.
{code}
for (final VertexGroupInfo groupInfo : vertexGroups.values()) {
if (!groupInfo.outputs.isEmpty()) {
- groupInfo.committed = true;
final Vertex v = getVertex(groupInfo.groupMembers.iterator().next());
for (final String outputName : groupInfo.outputs) {
final OutputKey outputKey = new OutputKey(outputName,
groupInfo.groupName, true);
@@ -1920,7 +1931,6 @@ public class DAGImpl implements
org.apache.tez.dag.app.dag.DAG,
+ " data, groupName=" + groupInfo.groupName);
continue;
}
- groupInfo.committed = true;{code}
This is probably not going to work with the above code
{code} // partial output may already have been in committing or committed.
fail if so
List<VertexGroupInfo> groupList = vertexGroupInfo.get(vertex.getName());
if (groupList != null) {
for (VertexGroupInfo groupInfo : groupList) {
if (groupInfo.isInCommitting()) {
String msg = "Aborting job as committing vertex: "
+ vertex.getLogIdentifier() + " is re-running";
LOG.info(msg);
addDiagnostic(msg);
enactKill(DAGTerminationCause.VERTEX_RERUN_IN_COMMITTING,
VertexTerminationCause.VERTEX_RERUN_IN_COMMITTING);
return true;
} else if (groupInfo.isCommitted()) {{code}
1) succeededCommits looks unused - we could remove it
2) Why is vertexGroup.commitStarted=true here? this is where commit finishes,
right?
3) if condition can be replaced by vertexGroup.isCommitted();
4) unnecessary space before ++
5) missing { after if stmt
{code}
+ OutputKey outputKey = commitCompletedEvent.getOutputKey();
+ succeededCommits.add(outputKey); <<<<<<< unused
+ if (outputKey.isVertexGroupOutput){
+ VertexGroupInfo vertexGroup =
vertexGroups.get(outputKey.getEntityName());
+ vertexGroup.commitStarted = true; <<<<<<< why here at finish time?
+ vertexGroup.successfulCommits ++; <<<<<< space
+ if (vertexGroup.successfulCommits == vertexGroup.outputs.size()) {
<<<<<< replace with isCommitted()
+ if (!commitAllOutputsOnSuccess) <<<<<<<<< missing {
+ try {
{code}
Which test case is covered the VertexImpl change?
testVertexCommit_OnVertexSuccess()?
Which test/check is covering that vertexgroupcommit event is not written for a
non-group vertex when all commits happen on dag success?
Rename testVertexSucceed_OnDAGSuccess() to testVertexCommit_OnDAGSuccess()?
> VertexGroupCommitFinishedEvent & VertexCommitStartedEvent is not logged
> correctly
> ---------------------------------------------------------------------------------
>
> Key: TEZ-2410
> URL: https://issues.apache.org/jira/browse/TEZ-2410
> Project: Apache Tez
> Issue Type: Bug
> Affects Versions: 0.7.0
> Reporter: Jeff Zhang
> Assignee: Jeff Zhang
> Priority: Blocker
> Attachments: TEZ-2410-1.patch, TEZ-2410-1.patch, TEZ-2410-2.patch
>
>
> VertexGroupCommitFinishedEvent may be logged for non-vertex group commits.
> VertexGroupCommitFinishedEvent may be logged for each member vertex of the
> group instead of once per group.
> VertexCommitStartedEvent may be logged for each output of vertex
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
