abstractdog commented on code in PR #6436:
URL: https://github.com/apache/hive/pull/6436#discussion_r3098322784
##########
llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/LlapTaskSchedulerService.java:
##########
@@ -454,9 +454,9 @@ public LlapTaskSchedulerService(TaskSchedulerContext
taskSchedulerContext, Clock
this.pauseMonitor = new JvmPauseMonitor(conf);
pauseMonitor.start();
String displayName = "LlapTaskSchedulerMetrics-" +
MetricsUtils.getHostName();
- String sessionId = conf.get("llap.daemon.metrics.sessionid");
- // TODO: Not sure about the use of this. Should we instead use
workerIdentity as sessionId?
- this.metrics = LlapTaskSchedulerMetrics.create(displayName, sessionId);
+ String metricsSessionId = conf.get("llap.daemon.metrics.sessionid");
Review Comment:
I believe we should not touch anything that's related to
`llap.daemon.metrics.sessionid` at this point, as this is just a docker testing
fix, and `conf.get("llap.daemon.metrics.sessionid")` means it can be configured
anytime by client payload: even I cannot find a reference to it in e.g.
Cloudera products, theoretically, it can be used, so I believe
[HIVE-29569](https://issues.apache.org/jira/browse/HIVE-29569) is the place
where we can do it
the only question for me is whether I should keep it as `metricsSessionId`
for clarification, or simply revert to `sessionId` to make this patch simpler
considering your approvals guys, I'm assuming this patch fine, and waiting a
few hours before proceeding to merge
--
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]
