phet commented on code in PR #3896:
URL: https://github.com/apache/gobblin/pull/3896#discussion_r1540534773
##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/KafkaJobStatusMonitor.java:
##########
@@ -193,7 +194,7 @@ protected void
processMessage(DecodeableKafkaRecord<byte[],byte[]> message) {
org.apache.gobblin.configuration.State jobStatus =
parseJobStatus(gobblinTrackingEvent);
if (jobStatus != null) {
try (Timer.Context context =
getMetricContext().timer(GET_AND_SET_JOB_STATUS).time()) {
- addJobStatusToStateStore(jobStatus, this.stateStore,
this.eventProducer, this.dagActionStore);
+ addJobStatusToStateStore(jobStatus, this.stateStore,
this.eventProducer, this.dagActionStore, this.dagProcEngineEnabled);
Review Comment:
rather than pass one more param to this `static` which appears to have been
designed in the main to be `@VisibleForTesting`, I had suggested passing in two
fewer params, `this.eventProducer` and `this.dagActionStore`. then have it
return `Optional<JobStatus>`, present only when the job state newly
transitioned to final.
the resulting `static` method would be much more cohesive (and testable!),
as it solely updates the job state store, without performing side-effects
anywhere else
--
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]
