You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/11/30 13:31:31 UTC
[GitHub] [hive] vcsomor opened a new pull request #2827: Hive 25737 fix cycle metrics
vcsomor opened a new pull request #2827:
URL: https://github.com/apache/hive/pull/2827
This PR is about to improve the Compaction Initiator/Worker/Cleaner cycle metrics.
However the original PerformanceLogger approach is kept for the Initiator/Worker/Cleaner cycle metrics three new metric has been added.
## Initiator duration:
A daemon thread has been implemented for the Initiator, that measures the elapsed time since its start.
## Age of the oldest working compaction
The Age of Oldest Working Compaction metric has been implemented in the AcidMetricService.
## Age of Oldest active Cleaner metric
- CQ_CLEANER_START field added to the COMPACTION_QUEUE table
- COMPACTION_OLDEST_CLEANING_AGE metric has been added
- markCleaning method added to the CompactionTxnHandler
- ShowCompactResponseElement extended with the cleanerStart field
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] lcspinter commented on a change in pull request #2827: Hive 25737 fix cycle metrics
Posted by GitBox <gi...@apache.org>.
lcspinter commented on a change in pull request #2827:
URL: https://github.com/apache/hive/pull/2827#discussion_r761742553
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CycleUpdaterThread.java
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import org.apache.hadoop.hive.metastore.metrics.Metrics;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This thread simply updates a gauge metric by the given interval with the time elapsed since start
+ */
+class CycleUpdaterThread extends Thread {
+
+ private static final String CLASS_NAME = CycleUpdaterThread.class.getName();
+ private static final Logger LOG = LoggerFactory.getLogger(CLASS_NAME);
+
+ private final String gaugeName;
+ private final long startTime;
+ private final long updateInterval;
+
+ /**
+ * Constructor
+ * @param gaugeName name of the gauge which obtained from {@link org.apache.hadoop.hive.metastore.metrics.Metrics}
+ * @param startTime the start of the measurement
+ * @param updateInterval the update interval
+ */
+ CycleUpdaterThread(String gaugeName, long startTime, long updateInterval) {
+ this.gaugeName = gaugeName;
+ this.startTime = startTime;
+ this.updateInterval = updateInterval;
+ }
+
+ @Override
+ public void run() {
+ while (!isInterrupted()) {
+ updateMetric();
+
+ try {
+ Thread.sleep(updateInterval);
+ } catch (InterruptedException e) {
+ LOG.debug("Thread has been interrupted - this is normal");
+ }
+ }
+ }
+
+ @Override
+ public void interrupt() {
+ LOG.debug("Interrupting for {}", gaugeName);
+ super.interrupt();
+ updateMetric();
+ }
+
+ private void updateMetric() {
+ updateMetric((int)(System.currentTimeMillis() - startTime));
+ }
+
+ private void updateMetric(int elapsed) {
Review comment:
Could you please combine the two updateMetric() functions?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CycleUpdaterThread.java
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import org.apache.hadoop.hive.metastore.metrics.Metrics;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This thread simply updates a gauge metric by the given interval with the time elapsed since start
+ */
+class CycleUpdaterThread extends Thread {
Review comment:
Although I like that thread and the threadfactory is separated into different classes, usually, I use the built-in java utilities for thread management. You might have a look at the `Executors` API. Also there is a `ThreadFactoryBuilder` which might substitute your `CycleUpdaterThreadFactory`.
A good example can be found in `CompactorUtil`.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java
##########
@@ -558,4 +563,11 @@ private String getInitiatorId(long threadId) {
name.append(threadId);
return name.toString();
}
+
+ private void stopCycleUpdaterThread() {
Review comment:
ExecutorService.shutdown()
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java
##########
@@ -25,7 +25,6 @@
import org.apache.hadoop.hive.common.ValidTxnList;
import org.apache.hadoop.hive.common.classification.RetrySemantics;
import org.apache.hadoop.hive.metastore.api.*;
Review comment:
I know it's not part of this change, but could you please replace this wildcard import?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CycleUpdaterThreadFactory.java
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+/**
+ * Factory for the {@link CycleUpdaterThread} class
+ */
+final class CycleUpdaterThreadFactory {
+
+ private static final long DEFAULT_UPDATE_INTERVAL_IN_MILLISECONDS = 1_000L;
Review comment:
Do we want to make this config costumisable?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] klcopp merged pull request #2827: Hive 25737 fix cycle metrics
Posted by GitBox <gi...@apache.org>.
klcopp merged pull request #2827:
URL: https://github.com/apache/hive/pull/2827
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] klcopp commented on a change in pull request #2827: Hive 25737 fix cycle metrics
Posted by GitBox <gi...@apache.org>.
klcopp commented on a change in pull request #2827:
URL: https://github.com/apache/hive/pull/2827#discussion_r763128180
##########
File path: common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
##########
@@ -3190,6 +3190,24 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
"has had a transaction done on it since the last major compaction. So decreasing this\n" +
"value will increase the load on the NameNode."),
+ HIVE_COMPACTOR_INITIATOR_DURATION_UPDATE_INTERVAL("hive.compactor.initiator.duration.update.interval", "1s",
Review comment:
I think the default update interval should be at least 10s, but probably more like 60s. There should be a balance of keeping users updated without burdening the HMS
##########
File path: standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
##########
@@ -494,6 +494,18 @@ public static ConfVars getMetaConf(String name) {
12, TimeUnit.HOURS,
"Age of oldest initiated compaction in the compaction queue after which an error will be logged. " +
"Default time unit is: hours"),
+ COMPACTOR_LONG_RUNNING_INITIATOR_THRESHOLD_WARNING(
+ "metastore.compactor.long.running.initiator.threshold.warning",
+ "hive.compactor.long.running.initiator.threshold.warning",
+ 6, TimeUnit.HOURS,
+ "Initiator duration after which a warning will be logged. " +
+ "Default time unit is: hours"),
+ COMPACTOR_LONG_RUNNING_INITIATOR_THRESHOLD_ERROR(
+ "metastore.compactor.long.running.initiator.threshold.error",
+ "hive.compactor.long.running.initiator.threshold.error",
+ 12, TimeUnit.HOURS,
+ "Initiator duration after which an wrror will be logged. " +
Review comment:
nit: spelling
##########
File path: common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
##########
@@ -3190,6 +3190,24 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
"has had a transaction done on it since the last major compaction. So decreasing this\n" +
"value will increase the load on the NameNode."),
+ HIVE_COMPACTOR_INITIATOR_DURATION_UPDATE_INTERVAL("hive.compactor.initiator.duration.update.interval", "1s",
+ new TimeValidator(TimeUnit.SECONDS),
+ "Time in seconds that drives the update interval of compaction_initiator_duration metric.\n" +
+ "Smaller value results in a fine grained metric update.\n" +
+ "This updater can be turned off if its value less than or equals to zero.\n"+
+ "In this case the above metric will be update only after the initiator completed one cycle.\n" +
+ "The hive.compactor.initiator.on must be turned on (true) in-order to enable the Initiator,\n" +
+ "otherwise this setting has no effect."),
+
+ HIVE_COMPACTOR_CLEANER_DURATION_UPDATE_INTERVAL("hive.compactor.cleaner.duration.update.interval", "1s",
+ new TimeValidator(TimeUnit.SECONDS),
+ "Time in seconds that drives the update interval of compaction_cleaner_duration metric.\n" +
+ "Smaller value results in a fine grained metric update.\n" +
+ "This updater can be turned off if its value less than or equals to zero.\n"+
+ "In this case the above metric will be update only after the initiator completed one cycle.\n" +
Review comment:
nit: Initiator is mentioned here and in the lines below.
##########
File path: standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
##########
@@ -494,6 +494,18 @@ public static ConfVars getMetaConf(String name) {
12, TimeUnit.HOURS,
"Age of oldest initiated compaction in the compaction queue after which an error will be logged. " +
"Default time unit is: hours"),
+ COMPACTOR_LONG_RUNNING_INITIATOR_THRESHOLD_WARNING(
+ "metastore.compactor.long.running.initiator.threshold.warning",
+ "hive.compactor.long.running.initiator.threshold.warning",
+ 6, TimeUnit.HOURS,
+ "Initiator duration after which a warning will be logged. " +
Review comment:
I would say, "Initiator cycle duration" here just to be clear that it's the whole cycle we're talking about
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] lcspinter commented on a change in pull request #2827: Hive 25737 fix cycle metrics
Posted by GitBox <gi...@apache.org>.
lcspinter commented on a change in pull request #2827:
URL: https://github.com/apache/hive/pull/2827#discussion_r761936734
##########
File path: common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
##########
@@ -3190,6 +3190,11 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
"has had a transaction done on it since the last major compaction. So decreasing this\n" +
"value will increase the load on the NameNode."),
+ HIVE_COMPACTOR_INITIATOR_DURATION_UPDATE_INTERVAL("hive.compactor.initiator.duration.update.interval", "1s",
+ new TimeValidator(TimeUnit.SECONDS),
+ "Time in seconds that drives the update interval of compaction_initiator_duration metric.\n" +
+ "Smaller value results in a fine grained metric update.\n"),
Review comment:
Do you mind adding a sentence here, what will happen if this value is negative? Please mention as well, that `hive.compactor.initiator.on` must be set to `true` to make this work.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] vcsomor commented on a change in pull request #2827: Hive 25737 fix cycle metrics
Posted by GitBox <gi...@apache.org>.
vcsomor commented on a change in pull request #2827:
URL: https://github.com/apache/hive/pull/2827#discussion_r761932541
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CycleUpdaterThread.java
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+import org.apache.hadoop.hive.metastore.metrics.Metrics;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This thread simply updates a gauge metric by the given interval with the time elapsed since start
+ */
+class CycleUpdaterThread extends Thread {
Review comment:
Okay... I've changed the implementation to that way
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] vcsomor commented on a change in pull request #2827: Hive 25737 fix cycle metrics
Posted by GitBox <gi...@apache.org>.
vcsomor commented on a change in pull request #2827:
URL: https://github.com/apache/hive/pull/2827#discussion_r761797796
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/CycleUpdaterThreadFactory.java
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.ql.txn.compactor;
+
+/**
+ * Factory for the {@link CycleUpdaterThread} class
+ */
+final class CycleUpdaterThreadFactory {
+
+ private static final long DEFAULT_UPDATE_INTERVAL_IN_MILLISECONDS = 1_000L;
Review comment:
I was thinking on this while implementing... I can do that
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] klcopp commented on a change in pull request #2827: Hive 25737 fix cycle metrics
Posted by GitBox <gi...@apache.org>.
klcopp commented on a change in pull request #2827:
URL: https://github.com/apache/hive/pull/2827#discussion_r763768080
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/MetricsConstants.java
##########
@@ -22,9 +22,13 @@
public static final String API_PREFIX = "api_";
public static final String COMPACTION_STATUS_PREFIX = "compaction_num_";
public static final String COMPACTION_OLDEST_ENQUEUE_AGE = "compaction_oldest_enqueue_age_in_sec";
+ public static final String COMPACTION_OLDEST_WORKING_AGE = "compaction_oldest_working_age_in_sec";
+ public static final String COMPACTION_OLDEST_CLEANING_AGE = "compaction_oldest_cleaning_age_in_sec";
public static final String COMPACTION_INITIATOR_CYCLE = "compaction_initiator_cycle";
+ public static final String COMPACTION_INITIATOR_DURATION = "compaction_initiator_duration";
Review comment:
I would change this to compaction_initiator_cycle_duration in order to be very clear; similar for compaction_cleaner_duration
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -379,6 +379,102 @@ public void markCompacted(CompactionInfo info) throws MetaException {
}
}
+ /**
+ * Mark the cleaning start time for a particular compaction
+ *
+ * @param info info on the compaction entry
+ */
+ @Override
+ @RetrySemantics.CannotRetry
Review comment:
I'm not 100% on these, but shouldn't this have the @RetrySemantics.ReadOnly annotation?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org