tez-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bikas Saha (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (TEZ-1547) Make use of state change notifier in VertexManagerPlugins
Date Fri, 31 Oct 2014 23:01:34 GMT

    [ https://issues.apache.org/jira/browse/TEZ-1547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14192318#comment-14192318
] 

Bikas Saha edited comment on TEZ-1547 at 10/31/14 11:01 PM:
------------------------------------------------------------

bq. I think it'll be better to send this out as early as possible if it's really a reconfiguration
update
The difference between setParallelism and startVertex is 2 events on the dispatcher queue
that are sent back to back. setParallelism->immediately sends READY_TO_INIT-> immediately
triggers START-> sends CONFIGURED_COMPLETE. So time difference would be negligibly small.
Sending it in startVertex makes it future proof to any code changes prior to start.

bq. Also, should we drop PARALLELISM_UPDATED - since that's a subset of RECONFIGURED
IMO parallelism updated is useful by itself, specially when parallelism may be updated multiple
times. It can also be used to split downstream vertices on 1-1 edges instead of the current
one-off implementation to do so. I had initially thought of reconfigured but its not necessary
that the vertex was reconfigured in many case and so the name would be misleading.

bq. It's still possible for COMPLETELY_CONFIGURED to go out twice 
Both are under the writeLock. So they cannot interleave. If done() happens first then it will
fail the startTime check and no event will be sent. Then startVertex will send the event because
byManager==false. If start happens first then event will not be sent since byManager==true.
When done() is called then it will see startTime>0 and send the event once. In anycase,
I can use an AtomicBoolean to make things really clear.

bq. if (fromVertexManager && canInitVertex()) - Should probably be after the if (parallelismSet
check)
Will do.

bq. VertexManagerPlugin.onVertexStateUpdated (and InputInitializerPluginContext.onVertexStateUpdated)
- JavaDoc should ideally mention
Will add.

bq. ImmediateStartVertexManager. Does it make sense to rename these
Will let the names stay since the behavior to start asap is still there.

bq. if (getContext().getVertexNumTasks(srcVertex) > 0)
Not having the check will work but I kept the original code because 0 task vertices dont really
need to be tracked.

bq. if (srcVertexConfigured.put(stateUpdate.getVertexName(), true).booleanValue() == false)
{
Duplicates should not occur. Will change that to a Precondition.

bq. Exception handling change in VertexImpl - the Vertex should move into FAILED state instead
of KILELD
Will do

bq. VertexManagerUserCodeErrorTransition needs to handle different states - RUNNING (tasks
scheduled) vs INITED/INITING
Will do.

bq. If leaving them in place, please add a comment on the variable indicating eventual intent,
for folks browsing the code.
Will do


was (Author: bikassaha):
bq. I think it'll be better to send this out as early as possible if it's really a reconfiguration
update
The difference between setParallelism and startVertex is 2 events on the dispatcher queue
that are sent back to back. setParallelism->immediately sends READY_TO_INIT-> immediately
triggers START-> sends CONFIGURED_COMPLETE. So time difference would be negligibly small.
Sending it in startVertex makes it future proof to any code changes prior to start.

bq. Also, should we drop PARALLELISM_UPDATED - since that's a subset of RECONFIGURED
IMO parallelism updated is useful by itself, specially when parallelism may be updated multiple
times. It can also be used to split downstream vertices on 1-1 edges instead of the current
one-off implementation to do so. I had initially thought of reconfigured but its not necessary
that the vertex was reconfigured in many case and so the name would be misleading.

bq. It's still possible for COMPLETELY_CONFIGURED to go out twice 
Will fix.

bq. if (fromVertexManager && canInitVertex()) - Should probably be after the if (parallelismSet
check)
Will do.

bq. VertexManagerPlugin.onVertexStateUpdated (and InputInitializerPluginContext.onVertexStateUpdated)
- JavaDoc should ideally mention
Will add.

bq. ImmediateStartVertexManager. Does it make sense to rename these
Will let the names stay since the behavior to start asap is still there.

bq. if (getContext().getVertexNumTasks(srcVertex) > 0)
Not having the check will work but I kept the original code because 0 task vertices dont really
need to be tracked.

bq. if (srcVertexConfigured.put(stateUpdate.getVertexName(), true).booleanValue() == false)
{
Duplicates should not occur. Will change that to a Precondition.

bq. Exception handling change in VertexImpl - the Vertex should move into FAILED state instead
of KILELD
Will do

bq. VertexManagerUserCodeErrorTransition needs to handle different states - RUNNING (tasks
scheduled) vs INITED/INITING
Will do.

bq. If leaving them in place, please add a comment on the variable indicating eventual intent,
for folks browsing the code.
Will do

> Make use of state change notifier in VertexManagerPlugins
> ---------------------------------------------------------
>
>                 Key: TEZ-1547
>                 URL: https://issues.apache.org/jira/browse/TEZ-1547
>             Project: Apache Tez
>          Issue Type: Improvement
>            Reporter: Siddharth Seth
>            Assignee: Siddharth Seth
>         Attachments: TEZ-1547.1.patch, TEZ-1547.3.patch, TEZ-1547.4.patch, TEZ-1547.5.patch,
TEZ-1547.6.patch, TEZ-1547.7.patch, TEZ-1547.8.patch
>
>
> Instead of the various APIs like onVertexStarted, simple notifications could be sent.
> Some existing APIs could end up being deprecated.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message