autumnust commented on a change in pull request #3365:
URL: https://github.com/apache/gobblin/pull/3365#discussion_r689834209
##########
File path:
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTaskStateTracker.java
##########
@@ -61,7 +61,9 @@ public GobblinHelixTaskStateTracker(Properties properties) {
@Override
public void registerNewTask(Task task) {
try {
- this.scheduledReporters.put(task.getTaskId(),
scheduleTaskMetricsUpdater(new TaskMetricsUpdater(task), task));
+ if (GobblinMetrics.isEnabled(task.getTaskState().getWorkunit())) {
+ this.scheduledReporters.put(task.getTaskId(),
scheduleTaskMetricsUpdater(new TaskMetricsUpdater(task), task));
Review comment:
the additional thing being placed within the condition of
`metrics.enabled` to be true, a.k.a `TaskMetricsUpdater` seems to be pretty
metrics related and lightweight as it calls `updateRecordMetrics` and
`updateByteMetrics`. Both of them are included if instrumented writer is being
used, so I believe it is fair to gate these behavior with this config.
If it is reversed, i.e. the `metrics.enabled` is controlling lightweight
metrics-reporting behavior while we are trying to let it control the emission
of GTE then I will be concerned as well. Let me know if it makes sense.
--
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]
