abstractdog commented on code in PR #6659:
URL: https://github.com/apache/hive/pull/6659#discussion_r3680676973
##########
llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/LlapTaskSchedulerService.java:
##########
@@ -470,15 +472,15 @@ public LlapTaskSchedulerService(TaskSchedulerContext
taskSchedulerContext, Clock
this.workloadManagementEnabled =
!StringUtils.isEmpty(conf.get(ConfVars.HIVE_SERVER2_TEZ_INTERACTIVE_QUEUE.varname,
"").trim());
+ ApplicationAttemptId appAttemptId = getContext().getApplicationAttemptId();
synchronized (LlapTaskCommunicator.pluginInitLock) {
- LlapTaskCommunicator peer = LlapTaskCommunicator.instance;
+ LlapTaskCommunicator peer =
LlapTaskCommunicator.pendingCommunicators.remove(appAttemptId);
if (peer != null) {
- // We are the last to initialize.
+ // We are the last to initialize for this DAG.
this.setTaskCommunicator(peer);
peer.setScheduler(this);
- LlapTaskCommunicator.instance = null;
} else {
- instance = this;
+ pendingSchedulers.put(appAttemptId, this);
}
Review Comment:
this makes sense to me
need to be aware that in production, we'll always have a single instance per
AM, but taking care of removing the instances properly in the shutdown() method
makes sense to me
however, this is not true: "a later communicator init to pair with a stopped
scheduler" <- this cannot happen, as both the schedulers and communicators are
keyed with ApplicationAttemptId, and every new JVM-local DAGAppMaster gets a
new application id...so the risk is not accidental re-pairing, just a simple
leak, which has to be avoided
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
