abstractdog commented on PR #6436:
URL: https://github.com/apache/hive/pull/6436#issuecomment-4260105614
> > @abstractdog , can you please help me in understanding this, I might be
wrong but won't this work, if we just overload the initSingleton and its
subsequent methods?
> > ```
> > diff --git
i/common/src/java/org/apache/hadoop/hive/common/JvmMetrics.java
w/common/src/java/org/apache/hadoop/hive/common/JvmMetrics.java
> > index fa0d285faf..7445bf2617 100644
> > --- i/common/src/java/org/apache/hadoop/hive/common/JvmMetrics.java
> > +++ w/common/src/java/org/apache/hadoop/hive/common/JvmMetrics.java
> > @@ -56,6 +56,13 @@ synchronized JvmMetrics init(String processName,
String sessionId) {
> > }
> > return impl;
> > }
> > +
> > + synchronized JvmMetrics init(String processName, String sessionId,
MetricsSystem ms) {
> > + if (impl == null) {
> > + impl = create(processName, sessionId, ms);
> > + }
> > + return impl;
> > + }
> > }
> >
> > static final float M = 1024*1024;
> > @@ -87,6 +94,10 @@ public static JvmMetrics initSingleton(String
processName, String sessionId) {
> > return Singleton.INSTANCE.init(processName, sessionId);
> > }
> >
> > + public static JvmMetrics initSingleton(String processName, String
sessionId, MetricsSystem ms) {
> > + return Singleton.INSTANCE.init(processName, sessionId, ms);
> > + }
> > +
> > @Override
> > public void getMetrics(MetricsCollector collector, boolean all) {
> > MetricsRecordBuilder rb = collector.addRecord(JvmMetrics)
> > diff --git
i/llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/metrics/LlapTaskSchedulerMetrics.java
w/llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/metrics/LlapTaskSchedulerMetrics.java
> > index 7518bae318..1eb9376408 100644
> > ---
i/llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/metrics/LlapTaskSchedulerMetrics.java
> > +++
w/llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/metrics/LlapTaskSchedulerMetrics.java
> > @@ -113,7 +113,7 @@ public static LlapTaskSchedulerMetrics create(String
displayName, String metrics
> > // Using different displayNames within the same JVM can still
trigger an issue even after fixing HIVE-29566,
> > // however it has not been observed in practice (including tests).
> > // This highlights the need to clean up this area.
> > - JvmMetrics jm =
JvmMetrics.create(MetricsUtils.METRICS_PROCESS_NAME, metricsSessionId, ms);
> > + JvmMetrics jm =
JvmMetrics.initSingleton(MetricsUtils.METRICS_PROCESS_NAME, metricsSessionId,
ms);
> > return ms.register(name, "Llap Task Scheduler Metrics",
> > new LlapTaskSchedulerMetrics(name, jm, metricsSessionId));
> > });
> > ```
>
> looks ok, but we could reuse now genric signature
>
> ```
> synchronized JvmMetrics init(String processName, String sessionId) {
> return init(processName, sessionId, DefaultMetricsSystem.instance());
> }
>
> synchronized JvmMetrics init(String processName, String sessionId,
MetricsSystem ms) {
> if (impl == null) {
> impl = create(processName, sessionId, ms);
> }
> return impl;
> }
> ```
>
> patch
>
> ```
> Subject: [PATCH] patch
> ---
> Index:
llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/metrics/LlapTaskSchedulerMetrics.java
> IDEA additional info:
> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
> <+>UTF-8
> ===================================================================
> diff --git
a/llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/metrics/LlapTaskSchedulerMetrics.java
b/llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/metrics/LlapTaskSchedulerMetrics.java
> ---
a/llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/metrics/LlapTaskSchedulerMetrics.java
(revision 2a38aab31b853f0ffdd64aeb5c185f8bf1c6bfcd)
> +++
b/llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/metrics/LlapTaskSchedulerMetrics.java
(date 1776340972465)
> @@ -31,6 +31,8 @@
> import static org.apache.hadoop.metrics2.impl.MsInfo.ProcessName;
> import static org.apache.hadoop.metrics2.impl.MsInfo.SessionId;
>
> +import java.util.concurrent.ConcurrentHashMap;
> +
> import org.apache.hadoop.hive.common.JvmMetrics;
> import org.apache.hadoop.hive.llap.metrics.LlapMetricsSystem;
> import org.apache.hadoop.hive.llap.metrics.MetricsUtils;
> @@ -51,6 +53,9 @@
> */
> @Metrics(about = "Llap Task Scheduler Metrics", context = "scheduler")
> public class LlapTaskSchedulerMetrics implements MetricsSource {
> + private static final ConcurrentHashMap<String,
LlapTaskSchedulerMetrics> METRICS =
> + new ConcurrentHashMap<>();
> +
> private final String name;
> private final JvmMetrics jvmMetrics;
> private final String sessionId;
> @@ -102,10 +107,12 @@
> }
>
> public static LlapTaskSchedulerMetrics create(String displayName,
String sessionId) {
> - MetricsSystem ms = LlapMetricsSystem.instance();
> - JvmMetrics jm = JvmMetrics.create(MetricsUtils.METRICS_PROCESS_NAME,
sessionId, ms);
> - return ms.register(displayName, "Llap Task Scheduler Metrics",
> - new LlapTaskSchedulerMetrics(displayName, jm, sessionId));
> + return METRICS.computeIfAbsent(displayName, name -> {
> + MetricsSystem ms = LlapMetricsSystem.instance();
> + JvmMetrics jm =
JvmMetrics.initSingleton(MetricsUtils.METRICS_PROCESS_NAME, sessionId, ms);
> + return ms.register(name, "Llap Task Scheduler Metrics",
> + new LlapTaskSchedulerMetrics(name, jm, sessionId));
> + });
> }
>
> @Override
> Index:
llap-server/src/java/org/apache/hadoop/hive/llap/metrics/LlapDaemonExecutorMetrics.java
> IDEA additional info:
> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
> <+>UTF-8
> ===================================================================
> diff --git
a/llap-server/src/java/org/apache/hadoop/hive/llap/metrics/LlapDaemonExecutorMetrics.java
b/llap-server/src/java/org/apache/hadoop/hive/llap/metrics/LlapDaemonExecutorMetrics.java
> ---
a/llap-server/src/java/org/apache/hadoop/hive/llap/metrics/LlapDaemonExecutorMetrics.java
(revision 2a38aab31b853f0ffdd64aeb5c185f8bf1c6bfcd)
> +++
b/llap-server/src/java/org/apache/hadoop/hive/llap/metrics/LlapDaemonExecutorMetrics.java
(date 1776341049814)
> @@ -236,7 +236,7 @@
> int numExecutorsConfigured, int waitQueueSizeConfigured, final
int[] intervals, int timedWindowAverageDataPoints,
> long timedWindowAverageWindowLength, int
simpleAverageWindowDataSize) {
> MetricsSystem ms = LlapMetricsSystem.instance();
> - JvmMetrics jm = JvmMetrics.create(MetricsUtils.METRICS_PROCESS_NAME,
sessionId, ms);
> + JvmMetrics jm =
JvmMetrics.initSingleton(MetricsUtils.METRICS_PROCESS_NAME, sessionId, ms);
> return ms.register(displayName, "LlapDaemon Executor Metrics",
> new LlapDaemonExecutorMetrics(displayName, jm, sessionId,
numExecutorsConfigured, waitQueueSizeConfigured,
> intervals, timedWindowAverageDataPoints,
timedWindowAverageWindowLength, simpleAverageWindowDataSize));
> Index: common/src/java/org/apache/hadoop/hive/common/JvmMetrics.java
> IDEA additional info:
> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
> <+>UTF-8
> ===================================================================
> diff --git a/common/src/java/org/apache/hadoop/hive/common/JvmMetrics.java
b/common/src/java/org/apache/hadoop/hive/common/JvmMetrics.java
> --- a/common/src/java/org/apache/hadoop/hive/common/JvmMetrics.java
(revision 2a38aab31b853f0ffdd64aeb5c185f8bf1c6bfcd)
> +++ b/common/src/java/org/apache/hadoop/hive/common/JvmMetrics.java
(date 1776340794035)
> @@ -51,8 +51,12 @@
> JvmMetrics impl;
>
> synchronized JvmMetrics init(String processName, String sessionId) {
> + return init(processName, sessionId,
DefaultMetricsSystem.instance());
> + }
> +
> + synchronized JvmMetrics init(String processName, String sessionId,
MetricsSystem ms) {
> if (impl == null) {
> - impl = create(processName, sessionId,
DefaultMetricsSystem.instance());
> + impl = create(processName, sessionId, ms);
> }
> return impl;
> }
> @@ -87,6 +91,10 @@
> return Singleton.INSTANCE.init(processName, sessionId);
> }
>
> + public static JvmMetrics initSingleton(String processName, String
sessionId, MetricsSystem ms) {
> + return Singleton.INSTANCE.init(processName, sessionId, ms);
> + }
> +
> @Override
> public void getMetrics(MetricsCollector collector, boolean all) {
> MetricsRecordBuilder rb = collector.addRecord(JvmMetrics)
> ```
took a quick look, I'm fine with this guy if you both can agree with this
approach, I'll apply it in this PR
--
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]
