tez-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Siddharth Seth (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (TEZ-1547) Make use of state change notifier in VertexManagerPlugins
Date Fri, 31 Oct 2014 09:17:33 GMT

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

Siddharth Seth commented on TEZ-1547:
-------------------------------------

bq. Its actually already done that way to make the common case easier. An incompletely configured
vertex will implicitly send the notification when its completely configured. Calling these
API's is optional for the common case. 
Yep. Was mentioning setParallelism as an alternate. I think the explicit approach is better.

bq. Done this way because there are many path out of INITING. But all of those immediately
go trigger startVertex(). So it was easier to put it there without any drawbacks. This would
result in this and RUNNING coming one after another except for cases like auto-reduce/multiple
changes in parallelism etc.
I think it'll be better to send this out as early as possible if it's really a reconfiguration
update - form within setParallelism itself (only way to configure a vertex), and onReconfiguringVertex
in case the user specifies intent, but doesn't actually call setParallelism.
In terms of the event name - RECONFIGURED would be better than COMPLETELY_CONFIGURED IMO.
COMPLETELY_CONFIGURED prevents adding support for re-configuration after a vertex has started.
While that's not planned at the moment, this at least doesn't get in the way of such a change.

Also, should we drop PARALLELISM_UPDATED - since that's a subset of RECONFIGURED.

The rest...

- It's still possible for COMPLETELY_CONFIGURED to go out twice (ShuffleVertexManger can generate
this). onVertexStarted, ShuffleVertexManager schedules and call doneWithReconf (event goes
out and sets flag to false). maybeSend.. will end up re-sending the event. VertexImpl should
guard against this.

- if (fromVertexManager && canInitVertex()) - Should probably be after the if (parallelismSet
check). Multiple calls to setParallelism surfacing as an error before an attempt to reconfigure
a configured vertex.

- In terms of the incompatible change mentioned in the previous comment - yep, MRInputSplitDistributor
users will be fine. I'd ideally like to confirm with Hive/Pig on their custom vertexManagers
and how they set up parallelism initially (-1, or a fixed value).

- VertexManagerPlugin.onVertexStateUpdated (and InputInitializerPluginContext.onVertexStateUpdated)
- JavaDoc should ideally mention that these events may come on a separate thread, and the
user code should account for this. Follow up jira.

- ImmediateStartVertexManager. Does it make sense to rename these (pre-commit) to something
like StartOnSourcesConfiguredVertexManager - since that's the current behaviour.

- "if (getContext().getVertexNumTasks(srcVertex) > 0)" - Is this because vertices with
parallelism set to -1 would end up not sending a SRC_VERTEX_STARTED event to downstream vertices,
and hence onVertexStarted isn't invoked ?

- "if (srcVertexConfigured.put(stateUpdate.getVertexName(), true).booleanValue() == false)
{" Is this required, or just guarding against duplicate notifications for the future?

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

- VertexManagerUserCodeErrorTransition needs to handle different states - RUNNING (tasks scheduled)
vs INITED/INITING. Also needs to call vertex.finished() - and send out an event to the DAG,
otherwise the DAG will end up hanging. Ref: RootInputInitFailedTransition

- VertexManager - the isComplete() checks are no longer required. It's harmless leaving them
there as long as we're mindful of what gets blocked eventually. If leaving them in place,
please add a comment on the variable indicating eventual intent, for folks browsing the code.

> 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: Bikas Saha
>         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