Copilot commented on code in PR #4173:
URL: https://github.com/apache/gobblin/pull/4173#discussion_r2912398684
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinClusterConfigurationKeys.java:
##########
@@ -154,6 +154,8 @@ public class GobblinClusterConfigurationKeys {
public static final String HELIX_JOB_STOP_TIMEOUT_SECONDS =
GOBBLIN_CLUSTER_PREFIX + "helix.job.stopTimeoutSeconds";
public static final long DEFAULT_HELIX_JOB_STOP_TIMEOUT_SECONDS = 10L;
Review Comment:
The surrounding code groups related constant pairs with blank lines between
different groups (see lines 149-150, 152-153, 155-156). The new constants
should be separated from the preceding `HELIX_JOB_STOP_TIMEOUT_SECONDS` group
by a blank line for consistency.
```suggestion
public static final long DEFAULT_HELIX_JOB_STOP_TIMEOUT_SECONDS = 10L;
```
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -415,22 +420,42 @@ public synchronized void stop() {
logger.info("Stopping the Gobblin Task runner");
- // Stop metric reporting
- if (this.containerMetrics.isPresent()) {
- this.containerMetrics.get().stopMetricsReporting();
- }
-
try {
stopServices();
} finally {
logger.info("All services are stopped.");
- this.taskStateModelFactory.shutdown();
+ shutdownTaskStateModelFactory();
disconnectHelixManager();
}
+ // Stop metric reporting only after tasks have completed and emitted their
final GTEs
+ if (this.containerMetrics.isPresent()) {
+ this.containerMetrics.get().stopMetricsReporting();
+ }
+
this.isStopped = true;
Review Comment:
Moving `stopMetricsReporting()` outside the `try/finally` block means it
will be skipped if `shutdownTaskStateModelFactory()` or
`disconnectHelixManager()` throws an unexpected runtime exception. In the
original code, metrics shutdown happened before the `try` block, so it was
always executed regardless of exceptions in the shutdown sequence.
To preserve the intent of this change (stop metrics after tasks complete)
while still ensuring metrics are always stopped, consider wrapping the
remainder of the `stop()` method in its own `try/finally`, e.g., move the
`stopMetricsReporting()` and `this.isStopped = true` into a `finally` block
that encompasses the current `try/finally`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
