[GitHub] [gobblin] phet commented on a diff in pull request #3623: [GOBBLIN-1764] Emit observability event

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

phet commented on code in PR #3623:
URL: https://github.com/apache/gobblin/pull/3623#discussion_r1070057390


##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/KafkaJobStatusMonitor.java:
##########
@@ -219,7 +229,8 @@ protected void processMessage(DecodeableKafkaRecord<byte[],byte[]> message) {
    * @throws IOException
    */
   @VisibleForTesting
-  static void addJobStatusToStateStore(org.apache.gobblin.configuration.State jobStatus, StateStore stateStore)
+  static void addJobStatusToStateStore(org.apache.gobblin.configuration.State jobStatus, StateStore stateStore,
+      Optional<GaaSObservabilityEventProducer> eventProducer)

Review Comment:
   I've not heard that debate....
   
   overall, the idea w/ `Optional` is to encode possible absence in the type system where it can be checked statically and automatically.  `null` is notoriously furtive... take for instance this very impl. (here), which does not check `stateStore != null`--but should it?  the practice I follow is that when that ought to be checked, the expectation should be stated, not in javadoc or other code comments, but via `Optional`.
   
   again, I've not heard specific concern w/ use as a parameter type.  quite the opposite, in fact: the use is noted to bring clear benefit.



-- 
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