[
https://issues.apache.org/jira/browse/TEZ-2003?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14692926#comment-14692926
]
Bikas Saha commented on TEZ-2003:
---------------------------------
All the new service plugins should be in runtime-api package where there rest
of the user defined plugins are currently placed.
They should not be in DAG API which deal with defining the DAG structure (as
opposed to pluggable user components)
bq. Special casing is in place primarily for LocalContainerExecutor, which
requires a bunch of information at runtime - which isn't needed in the context
otherwise. There's a jira to provide such information via runtime binding in
the payload. For the other cases, it's mainly used to make it simpler to write
tests - where the default executor can be easily overwritten for the tests. The
construction, along with the payload, remains the same - except it's direct
instead of using reflection.
bq. The 3 constructs are not used together everywhere. There's multiple events
/ other classes which only use a subset of these. A single class won't really
help there.
bq. executeInAm and executeInContainers are in Contexts to specify whether a
task runs in a service or in the AM. It's possible to set a DAG level default
to run everything in an external service, and some vertices either in
containers or in the AM. Similarly for the ServiceDescriptor - decide whether
the AM runs containers or uber-mode during setup.
bq. There's certain operations which are performed differently for local mode.
Also used to indicate to internal plugins whether they're running in local /
uber mode.
bq. To always run the YARNScheduler (i.e. register with YARN) if running in
non-local mode. If we were to support alternate frameworks, this could be
removed.
executeInContainer should be part of the default execution context that is
created by the user (for other frameworks) or created internally by TezClient
or the DAGAppMaster if no default is specified. That way things continue to run
on YARN as is.
I agree that local mode needs some internal details but that should be
restricted to just that and not spread everywhere.
For testing, we could create helpers that provide the service descriptor of the
YARN service plugin.
This change for hybrid execution is a fundamental and important change for Tez.
We must ensure that it flows elegantly and without leaky abstractions. I am
afraid the current spread of booleans and special casing can be error prone
where things can turn out to not work as expected. Where I would like to see
this is a place where scheduler/communicator/launcher/vertex etc. all just use
their specified plugins and there is a central place like TezClient or the
DAGAppMaster where defaults are setup to take care of backwards compatibility
(pre-plugin era) and internal-aware plugins like LocalMode. This is defensive
programming and will help us to easily make logical extensions of this hybrid
execution without more refactoring and cleaning up needed.
Similar creating a SchedulingPlugin object is an example of defensive
programming where we pass around this object which has semantic meaning instead
of passing around int types. Sure, entities which need 2 out of 3 can choose to
use only the getters of 2 out 3. But essentially tracking that object allows us
to clearly see which parts of the code/events are related to plugins and which
parts are not. Tracking ints does not provide that visibility.
There is a lot of code and context in this branch and looking at these changes
gives only a superficial understanding of all the changes. Hence my comments
are mainly structural because that is the level at which I can review this. I
am already losing context of the patch and the list of comments in my initial
comment :). However, I dont want to delay progress on the merge. So I am fine
with the current state as long as we agree that these structural changes would
be beneficial to the goal and future improvements to the hybrid execution
model. We can create follow up jiras for these and work on them post-merge.
These would not be critical to 0.8.0 because its all internal stuff but would
be important work items for 0.8.1.
An uber comment on uber mode :) is that it seems dangerous to run uber mode
tasks within the AM. Local mode is different because it use is primarily to
debug the code in an attached process debugger. But in production jobs, we
don't want to lose the AM session because a user code task crashed or leaked or
destroyed the AM JVM. Uber mode could instead create a child JVM within the AM
container right on startup and use that to run uber tasks. It would still avoid
the scheduling penalty for running 1 or 2 small tasks on YARN but keep the
session safe.
bq. It has to take a decision about running in local mode / non-local mode -
contexts can be null. They're not always setup and sent over the wire.
Until now, there wasnt any special logic in VertexImpl for local mode. The
decision for local mode is taken at the client and the vertex simply schedules
its task oblivous to where its running. So I dont understand why that has to
change. Decision about local/uber come from the client in the form of execution
contexts which the vertex simply passes on in the schedule task request.
bq. closely tied to execution environment and the current state of the code
with some bits handled and some bits not, can be error prone. This should be in
place already. TEZ-2124 is done.\
Not sure how this is fixed. Here is the code fragment from my initial comment.
In some places we are arbitrarily passing back schedulerId = 0.
{code}
/* Blacklist the node with the AMNodeTracker and check if the node should be
blacklisted */
protected boolean registerBadNodeAndShouldBlacklist() {
- return appContext.getNodeTracker().registerBadNodeAndShouldBlacklist(this);
+ return appContext.getNodeTracker().registerBadNodeAndShouldBlacklist(this,
sourceId);
}
@@ -257,7 +259,8 @@ public class AMNodeImpl implements AMNode {
- sendEvent(new AMSchedulerEventNodeBlacklistUpdate(getNodeId(), true));
+ // TODO TEZ-2124 node tracking per ext source
+ sendEvent(new AMSchedulerEventNodeBlacklistUpdate(getNodeId(), true, 0));
@@ -363,7 +366,8 @@ public class AMNodeImpl implements AMNode {
- node.sendEvent(new
AMSchedulerEventNodeBlacklistUpdate(node.getNodeId(), false));
+ // TODO TEZ-2124 node tracking per ext source
+ node.sendEvent(new
AMSchedulerEventNodeBlacklistUpdate(node.getNodeId(), false, 0));{code}
bq. numUpdate handling no longer done via TaskAttemptListener, so this isn't
needed.
Please take a look at the MockDAGAppMaster code. numUpdates is used internally
by that code. So the increment is still needed.
bq. This is again for transitions between DAGs. A new dag is received when a
dag is submitted - the context update needs to be factored out. dagComplete is
sent to a plugin - which can take an arbitrary time to process.
What will happen if the dag starts to run/launch new tasks while the
communicator is still procesing the completion of the previous dag? Say launch
on communicator will be invoked. To process the launch it may call getDAG()
which will then either return the wrong dag or stuck (or deadlocked?) behind
the dagChangedReadLock?
Not sure why SchedulerEvent/ContainerEvent base classes would cause
complications. Every scheduler event now needs a scheduler id. So every new
event needs to have that specified. So a base class that keeps that code in one
place sounds like a transparent change.
> [Umbrella] Allow Tez to co-ordinate execution to external services
> ------------------------------------------------------------------
>
> Key: TEZ-2003
> URL: https://issues.apache.org/jira/browse/TEZ-2003
> Project: Apache Tez
> Issue Type: Improvement
> Reporter: Siddharth Seth
> Attachments: 2003_20150728.1.txt, 2003_20150807.1.txt,
> 2003_20150807.2.txt, Tez With External Services.pdf
>
>
> The Tez engine itself takes care of co-ordinating execution - controlling how
> data gets routed (different connection patterns), fault tolerance, scheduling
> of work, etc.
> This is currently tied to TaskSpecs defined within Tez and on containers
> launched by Tez itself (TezChild).
> The proposal is to allow Tez to work with external services instead of just
> containers launched by Tez. This involves several more pluggable layers to
> work with alternate Task Specifications, custom launch and task allocation
> mechanics, as well as custom scheduling sources.
> A simple example would be a simple a process with the capability to execute
> multiple Tez TaskSpecs as threads. In such a case, a container launch isn't
> really need and can be mocked. Sourcing / scheduling containers would need to
> be pluggable.
> A more advanced example would be LLAP (HIVE-7926;
> https://issues.apache.org/jira/secure/attachment/12665704/LLAPdesigndocument.pdf).
> This works with custom interfaces - which would need to be supported by Tez,
> along with a custom event model which would need translation hooks.
> Tez should be able to work with a combination of certain vertices running in
> external services and others running in regular Tez containers.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
