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/06/09 13:28:14 UTC
[GitHub] [hive] asinkovits commented on a change in pull request #2332: HIVE-25081: Put metrics collection behind a feature flag
asinkovits commented on a change in pull request #2332:
URL: https://github.com/apache/hive/pull/2332#discussion_r648285067
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java
##########
@@ -111,7 +111,7 @@ public void run() {
// so wrap it in a big catch Throwable statement.
try {
handle = txnHandler.getMutexAPI().acquireLock(TxnStore.MUTEX_KEY.Cleaner.name());
- if (metricsEnabled) {
+ if (metricsEnabled && MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON)) {
Review comment:
yes. nice catch, fixed.
##########
File path: standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
##########
@@ -454,6 +454,8 @@ public static ConfVars getMetaConf(String name) {
"hive.metastore.acidmetrics.check.interval", 300,
TimeUnit.SECONDS,
"Time in seconds between acid related metric collection runs."),
+ METASTORE_ACIDMETRICS_EXT_ON("metastore.acidmetrics.ext.on", "hive.metastore.acidmetrics.ext.on", true,
+ "Whether to collect additional acid related metrics outside of the acid metrics service."),
Review comment:
And/Or HIVE_SERVER2_METRICS_ENABLED. Shall I add both?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java
##########
@@ -111,7 +111,7 @@ public void run() {
// so wrap it in a big catch Throwable statement.
try {
handle = txnHandler.getMutexAPI().acquireLock(TxnStore.MUTEX_KEY.Cleaner.name());
- if (metricsEnabled) {
+ if (metricsEnabled && MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON)) {
Review comment:
fixed.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/metrics/DeltaFilesMetricReporter.java
##########
@@ -115,41 +117,45 @@ public static DeltaFilesMetricReporter getInstance() {
return InstanceHolder.instance;
}
- public static synchronized void init(HiveConf conf){
+ public static synchronized void init(HiveConf conf) {
getInstance().configure(conf);
}
public void submit(TezCounters counters) {
- updateMetrics(NUM_OBSOLETE_DELTAS,
- obsoleteDeltaCache, obsoleteDeltaTopN, obsoleteDeltasThreshold, counters);
- updateMetrics(NUM_DELTAS,
- deltaCache, deltaTopN, deltasThreshold, counters);
- updateMetrics(NUM_SMALL_DELTAS,
- smallDeltaCache, smallDeltaTopN, deltasThreshold, counters);
+ if(acidMetricsExtEnabled) {
Review comment:
yeah.. So first issue is that this metrics is collected in HS2 but the conf (METASTORE_ACIDMETRICS_EXT_ON) for the metrics collection is defined in the MetastoreConf, so I wanted to minimize the exposure of that.
Second is in the end I put it here, because it seamed to make the class more resilient. Here is my reasoning: This is a singleton class, so you can access it basically anywhere, but you need to call the init method on it so that it works properly. The acidMetricsExtEnabled flag is indeed set in the init (configure), so if it was not called it will skip this code part which otherwise would throw a NPE.
But I'm open to a better solution :)
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/metrics/DeltaFilesMetricReporter.java
##########
@@ -115,41 +117,45 @@ public static DeltaFilesMetricReporter getInstance() {
return InstanceHolder.instance;
}
- public static synchronized void init(HiveConf conf){
+ public static synchronized void init(HiveConf conf) {
getInstance().configure(conf);
}
public void submit(TezCounters counters) {
- updateMetrics(NUM_OBSOLETE_DELTAS,
- obsoleteDeltaCache, obsoleteDeltaTopN, obsoleteDeltasThreshold, counters);
- updateMetrics(NUM_DELTAS,
- deltaCache, deltaTopN, deltasThreshold, counters);
- updateMetrics(NUM_SMALL_DELTAS,
- smallDeltaCache, smallDeltaTopN, deltasThreshold, counters);
+ if(acidMetricsExtEnabled) {
+ updateMetrics(NUM_OBSOLETE_DELTAS,
+ obsoleteDeltaCache, obsoleteDeltaTopN, obsoleteDeltasThreshold, counters);
+ updateMetrics(NUM_DELTAS,
+ deltaCache, deltaTopN, deltasThreshold, counters);
+ updateMetrics(NUM_SMALL_DELTAS,
+ smallDeltaCache, smallDeltaTopN, deltasThreshold, counters);
+ }
}
- public static void mergeDeltaFilesStats(AcidDirectory dir, long checkThresholdInSec,
- float deltaPctThreshold, EnumMap<DeltaFilesMetricType, Map<String, Integer>> deltaFilesStats) throws IOException {
- long baseSize = getBaseSize(dir);
- int numObsoleteDeltas = getNumObsoleteDeltas(dir, checkThresholdInSec);
+ public static void mergeDeltaFilesStats(AcidDirectory dir, long checkThresholdInSec, float deltaPctThreshold,
+ EnumMap<DeltaFilesMetricType, Map<String, Integer>> deltaFilesStats, Configuration conf) throws IOException {
+ if (MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON)) {
Review comment:
This is to minimize the exposure of the MetastoreConf. I've tried to put as many checks into this class as I could.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/metrics/DeltaFilesMetricReporter.java
##########
@@ -206,7 +216,7 @@ public static void backPropagateAcidMetrics(JobConf jobConf, Configuration conf)
}
public static void close() {
- if (getInstance() != null) {
+ if (getInstance() != null && getInstance().acidMetricsExtEnabled) {
Review comment:
the executorService is created in the configure method. So if it was not called this will throw a NPE if I understand.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java
##########
@@ -120,7 +120,7 @@ public void run() {
// don't doom the entire thread.
try {
handle = txnHandler.getMutexAPI().acquireLock(TxnStore.MUTEX_KEY.Initiator.name());
- if (metricsEnabled) {
+ if (metricsEnabled && MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON)) {
Review comment:
fixed.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/metrics/DeltaFilesMetricReporter.java
##########
@@ -190,13 +197,16 @@ public static void createCountersForAcidMetrics(TezCounters tezCounters, JobConf
}
public static void addAcidMetricsToConfObj(EnumMap<DeltaFilesMetricType, Map<String, Integer>> deltaFilesStats, Configuration conf) {
- deltaFilesStats.forEach((type, value) ->
- conf.set(type.name(), Joiner.on(",").withKeyValueSeparator("->").join(value))
- );
+ if (MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON)) {
Review comment:
nice, fixed.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/metrics/DeltaFilesMetricReporter.java
##########
@@ -230,23 +240,26 @@ private static long getDirSize(AcidUtils.ParsedDirectory dir, FileSystem fs) thr
.sum();
}
- private void configure(HiveConf conf){
- deltasThreshold = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVE_TXN_ACID_METRICS_DELTA_NUM_THRESHOLD);
- obsoleteDeltasThreshold = HiveConf.getIntVar(conf, HiveConf.ConfVars.HIVE_TXN_ACID_METRICS_OBSOLETE_DELTA_NUM_THRESHOLD);
-
- initMetricsCache(conf);
- long reportingInterval = HiveConf.getTimeVar(conf,
- HiveConf.ConfVars.HIVE_TXN_ACID_METRICS_REPORTING_INTERVAL, TimeUnit.SECONDS);
-
- ThreadFactory threadFactory =
- new ThreadFactoryBuilder()
- .setDaemon(true)
- .setNameFormat("DeltaFilesMetricReporter %d")
- .build();
- executorService = Executors.newSingleThreadScheduledExecutor(threadFactory);
- executorService.scheduleAtFixedRate(
- new ReportingTask(), 0, reportingInterval, TimeUnit.SECONDS);
- LOG.info("Started DeltaFilesMetricReporter thread");
+ private void configure(HiveConf conf) {
+ acidMetricsExtEnabled = MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON);
+ if (acidMetricsExtEnabled) {
Review comment:
Again exposure of the MetastoreConf. It might not be a valid reason though... :D
And because of my comment in the close method ("the executorService is created in the configure method.")
we need to track the acidMetricsExtEnabled in this class.
--
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.
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