[GitHub] [gobblin] aplex commented on a change in pull request #3350: [GOBBLIN-1504] add a flag to indicate the start of cluster manager

[GitBox <gi...@apache.org>]

aplex commented on a change in pull request #3350:
URL: https://github.com/apache/gobblin/pull/3350#discussion_r685504917



##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinClusterManager.java
##########
@@ -135,6 +135,8 @@
   private GobblinHelixJobScheduler jobScheduler;
   @Getter
   private JobConfigurationManager jobConfigurationManager;
+  @Getter
+  private volatile boolean started = false;

Review comment:
       So how is this flag going to be used? I see that it is only set to true, but never read in OSS codebase. 

##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/job_monitor/KafkaJobMonitor.java
##########
@@ -79,8 +85,11 @@ public KafkaJobMonitor(String topic, MutableJobCatalog catalog, Config config) {
   @Override
   protected void createMetrics() {
     super.createMetrics();
-    this.newSpecs = this.getMetricContext().counter(RuntimeMetrics.GOBBLIN_JOB_MONITOR_KAFKA_NEW_SPECS);
-    this.removedSpecs = this.getMetricContext().counter(RuntimeMetrics.GOBBLIN_JOB_MONITOR_KAFKA_REMOVED_SPECS);
+    this.newSpecs = this.getMetricContext().contextAwareMeter(RuntimeMetrics.GOBBLIN_JOB_MONITOR_KAFKA_NEW_SPECS);
+    this.updatedSpecs = this.getMetricContext().contextAwareMeter(RuntimeMetrics.GOBBLIN_JOB_MONITOR_KAFKA_UPDATED_SPECS);
+    this.removedSpecs = this.getMetricContext().contextAwareMeter(RuntimeMetrics.GOBBLIN_JOB_MONITOR_KAFKA_REMOVED_SPECS);
+    this.cancelledSpecs = this.getMetricContext().contextAwareMeter(RuntimeMetrics.GOBBLIN_JOB_MONITOR_KAFKA_CANCELLED_SPECS);
+    this.consumedSpecs = this.getMetricContext().contextAwareMeter(RuntimeMetrics.GOBBLIN_JOB_MONITOR_KAFKA_TOTAL_SPECS);

Review comment:
       Is it "total" or "consumed"? Better to have the same term throughout the code base.

##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinClusterManager.java
##########
@@ -218,6 +220,7 @@ private void startAppLauncherAndServices() {
     }
 
     this.applicationLauncher.start();
+    this.started = true;

Review comment:
       Does it makes sense to set it at the end of the "start" method? It's not obvious that this function will be called last on the start, and this sequence can change in the future.




-- 
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: dev-unsubscribe@gobblin.apache.org

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