[
https://issues.apache.org/jira/browse/GOBBLIN-1764?focusedWorklogId=839186&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-839186
]
ASF GitHub Bot logged work on GOBBLIN-1764:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 13/Jan/23 23:04
Start Date: 13/Jan/23 23:04
Worklog Time Spent: 10m
Work Description: homatthew commented on code in PR #3623:
URL: https://github.com/apache/gobblin/pull/3623#discussion_r1070151228
##########
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 don't have a strong preference for one or the other as long as the team
agrees on a logically consistent pattern. This topic has come up in my past
roles, so I figured I'd ask out of curiosity. Good points about the API being
more self descriptive as opposed to an annotation like `@Nullable` which is
more hidden
The argument against `Optional` as as follows:
- WIthout optional, you can have 2 possible states `null` or actual reference
- With optional, you can have 3 possible states `null`, `Optional.empty()`,
`Optional.present()`
It's reasonable to try your best not to pass null as a parameter. (e.g.
Optional.ofNullable(...)). But it does add visual clutter from a readability
perspective that doesn't add much value when method overloading would be
totally sufficient.
This is especially true when there is only one optional parameter. Here, we
don't have a bunch of permutations of options that the method caller could use
(Which is probably a code smell on its own).
Some debates in case you want to go down the rabbit hole / get nerd sniped:
https://rules.sonarsource.com/java/RSPEC-3553
https://stackoverflow.com/questions/31922866/why-should-java-8s-optional-not-be-used-in-arguments
http://dolszewski.com/java/java-8-optional-use-cases/
https://stackoverflow.com/questions/26327957/should-java-8-getters-return-optional-type/26328555#26328555
https://docs.oracle.com/javase/10/docs/api/java/util/Optional.html
> Optional is primarily intended for use as a method return type where there
is a clear need to represent "no result," and where using null is likely to
cause errors. A variable whose type is Optional should never itself be null; it
should always point to an Optional instance.
Issue Time Tracking
-------------------
Worklog Id: (was: 839186)
Time Spent: 2h (was: 1h 50m)
> Emit GaaSObservabilityEvent
> ---------------------------
>
> Key: GOBBLIN-1764
> URL: https://issues.apache.org/jira/browse/GOBBLIN-1764
> Project: Apache Gobblin
> Issue Type: New Feature
> Components: gobblin-service
> Reporter: William Lo
> Assignee: Abhishek Tiwari
> Priority: Major
> Time Spent: 2h
> Remaining Estimate: 0h
>
> GaaSObservabilityEvents are a new events that provides a job summary from
> pipelines in GaaS. It differs from GobblinTrackingEvents as it runs once per
> job pipeline, and it intended to be easily queryable and alert on.
> We want to emit this observability event from GaaS by deriving it from a
> job's job status. Since this feature is Experimental and WIP, it is not
> expected to fill out all of the fields immediately.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
