phet commented on code in PR #3610:
URL: https://github.com/apache/gobblin/pull/3610#discussion_r1041819858
##########
gobblin-metrics-libs/gobblin-metrics-base/src/main/avro/GaaSObservabilityEventV0.avsc:
##########
@@ -0,0 +1,135 @@
+{
+ "type": "record",
+ "name": "GaaSObservabilityEventExperimental",
+ "namespace" : "org.apache.gobblin.metrics",
+ "doc": "An experimental feature for GaaS to emit events during and after a
job is executed.",
+ "fields": [
+ {
+ "name": "timestamp",
+ "type": "long",
+ "doc": "Time at which event was created in millis"
+ }, {
+ "name" : "flowGroup",
+ "type" : "string",
+ "doc" : "Flow group for the GaaS flow",
+ "compliance" : "NONE"
+ }, {
+ "name" : "flowName",
+ "type" : "string",
+ "doc" : "Flow name for the GaaS flow",
+ "compliance" : "NONE"
+ }, {
+ "name" : "flowExecutionId",
+ "type" : "long",
+ "doc" : "Flow execution id for the GaaS flow",
+ "compliance" : "NONE"
+ }, {
+ "name": "jobOrchestratedTime",
+ "type": "long",
+ "doc": "Timestamp when the job was successfully sent to the job
executor, -1 if it was unable to be sent."
Review Comment:
I prefer this to the boolean. and since now conceptually similar, I suggest
moving adjacent to the `jobStartTime` field.
also, as mentioned in reply to other comment, I prefer `null` to `-1`, so
unless you have strong pref, I recommend for the reason it gives notice when
the null check has been forgotten, e.g. when read in scala, where `["null",
"Foo"]` maps to `Option[Foo]`
##########
gobblin-metrics-libs/gobblin-metrics-base/src/main/avro/GaaSObservabilityEventV0.avsc:
##########
@@ -0,0 +1,135 @@
+{
+ "type": "record",
+ "name": "GaaSObservabilityEventExperimental",
+ "namespace" : "org.apache.gobblin.metrics",
+ "doc": "An experimental feature for GaaS to emit events during and after a
job is executed.",
+ "fields": [
+ {
+ "name": "timestamp",
+ "type": "long",
+ "doc": "Time at which event was created in millis"
+ }, {
+ "name" : "flowGroup",
+ "type" : "string",
+ "doc" : "Flow group for the GaaS flow",
+ "compliance" : "NONE"
+ }, {
+ "name" : "flowName",
+ "type" : "string",
+ "doc" : "Flow name for the GaaS flow",
+ "compliance" : "NONE"
+ }, {
+ "name" : "flowExecutionId",
+ "type" : "long",
+ "doc" : "Flow execution id for the GaaS flow",
+ "compliance" : "NONE"
+ }, {
+ "name": "jobOrchestratedTime",
+ "type": "long",
+ "doc": "Timestamp when the job was successfully sent to the job
executor, -1 if it was unable to be sent."
Review Comment:
I prefer this to the boolean. and since now conceptually similar, I suggest
moving adjacent to the `jobStartTime` field.
also, as mentioned in reply to other comment, I prefer `null` to `-1`, so
unless you have strong pref, I recommend for the reason it gives notice when
the null check has been forgotten, e.g. upon access in scala, where `["null",
"Foo"]` maps to `Option[Foo]`
--
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-unsubscr...@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
