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

2023-05-22 Thread via GitHub 


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


##
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:
   I added some helpers and documented that this is internal only.



-- 
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-unsubscr...@hadoop.apache.org

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


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

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

2023-05-12 Thread via GitHub 


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


##
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:
   If we keep it in the `Configuration` object then instead of a getter I could 
create a static method in `QueueMetrics` and use 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: common-issues-unsubscr...@hadoop.apache.org

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


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

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

2023-05-12 Thread via GitHub 


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


##
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:
   At first I put it in the `CapacitySchedulerConfiguration`, but then I 
realised that the `QueueMetrics` (not the `CSQueueMetrics`) should not depend 
on anything from CS.
   
   We don't really need a config property, and it should not be a public facing 
property either. But the Configuration object is shared and propagated 
everywhere were we need to check if we are in validation mode (`QueueMetrics` 
for the `PartitionQueueMetrics` and `CSQueueMetrics`). Not sure where else to 
propagate this flag, without introducing a dependency in `QueueMetrics` to 
`CapacitySechduler` related logic.



-- 
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-unsubscr...@hadoop.apache.org

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


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

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

2023-05-11 Thread via GitHub 


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


##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/QueueMetrics.java:
##
@@ -48,6 +48,7 @@
 import org.apache.hadoop.yarn.metrics.CustomResourceMetricValue;
 import 
org.apache.hadoop.yarn.server.resourcemanager.nodelabels.RMNodeLabelsManager;
 import org.apache.hadoop.yarn.server.resourcemanager.rmapp.RMAppState;
+import 
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration;

Review Comment:
   Moved to YarnConfiguration.



-- 
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-unsubscr...@hadoop.apache.org

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


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

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

2023-05-11 Thread via GitHub 


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


##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfigValidator.java:
##
@@ -56,8 +60,6 @@ public static boolean validateCSConfiguration(
   return true;
 } finally {
   newCs.stop();
-  QueueMetrics.clearQueueMetrics();
-  liveScheduler.resetSchedulerMetrics();

Review Comment:
   Note to reviewers: resetSchedulerMetrics does not contain any QueueMetrics, 
not sure why it was added in YARN-11211.



##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/QueueMetrics.java:
##
@@ -48,6 +48,7 @@
 import org.apache.hadoop.yarn.metrics.CustomResourceMetricValue;
 import 
org.apache.hadoop.yarn.server.resourcemanager.nodelabels.RMNodeLabelsManager;
 import org.apache.hadoop.yarn.server.resourcemanager.rmapp.RMAppState;
+import 
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration;

Review Comment:
   Note: I don't think the QueueMetrics should depend on capacity.anything, but 
I'm not sure where to put this config option.



-- 
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-unsubscr...@hadoop.apache.org

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


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