[GitHub] [hadoop] brumi1024 commented on a diff in pull request #5644: YARN-11490. Reverting YARN-11211 and eliminating the use of DefaultMetricsSystem during configuration validation

["brumi1024 (via GitHub)" <gi...@apache.org>]

brumi1024 commented on code in PR #5644:
URL: https://github.com/apache/hadoop/pull/5644#discussion_r1192439238


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/QueueMetrics.java:
##########
@@ -117,6 +117,8 @@ public class QueueMetrics implements MetricsSource {
   @Metric("Reserved CPU in virtual cores") MutableGaugeInt reservedVCores;
   @Metric("# of reserved containers") MutableGaugeInt reservedContainers;
 
+  public static final String CONFIGURATION_VALIDATION = "yarn.configuration-validation";

Review Comment:
   I would prefer having this in the CapacitySchedulerConfiguration object (possibly with getters and a short description what is it used for), preferably with the prefix of CS, as this is irrelevant in FS cases.
   
   Actually: do we need to have this as a config property? AFAIU we shouldn't set this via the xml, couldn't CS simply have a validation flag?



##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/QueueMetrics.java:
##########
@@ -334,13 +336,15 @@ public synchronized QueueMetrics getPartitionQueueMetrics(String partition) {
       QueueMetrics queueMetrics =
           new PartitionQueueMetrics(metricsSystem, this.queueName, parentQueue,
               this.enableUserMetrics, this.conf, partition);
-      registerMetrics(
+      metricsSystem.register(
           pSourceName(partitionJMXStr).append(qSourceName(this.queueName))
               .toString(),
           "Metrics for queue: " + this.queueName,
           queueMetrics.tag(PARTITION_INFO, partitionJMXStr).tag(QUEUE_INFO,
               this.queueName));
-      getQueueMetrics().put(metricName, queueMetrics);
+      if (!conf.getBoolean(CONFIGURATION_VALIDATION, false)) {

Review Comment:
   A getter with the default set in one place could be a bit safer.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org