phet commented on code in PR #3896:
URL: https://github.com/apache/gobblin/pull/3896#discussion_r1550342146
##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/KafkaJobStatusMonitor.java:
##########
@@ -236,8 +238,9 @@ protected void
processMessage(DecodeableKafkaRecord<byte[],byte[]> message) {
* It fills missing fields in job status and also merge the fields with the
* existing job status in the state store. Merging is required because we
* do not want to lose the information sent by other GobblinTrackingEvents.
- * Returns false if adding this state transitions the job status of the job
to final, otherwise returns false.
- * It will also return false if the job status was already final before
calling this method.
+ * Returns an absent Optional if adding this state transitions the job
status of the job to final, otherwise returns
+ * the updated job status wrapped inside an Optional.
+ * It will also return an absent Optional if the job status was already
final before calling this method.
Review Comment:
"exactly-once guarantee" might be clearest way to phrase.
with not choosing "at-least-once", we must consider what happens if this
method returns (and did successfully update the JobStateStore), but we fail
before completing the update to the DagActionStore.
seems the reevaluate action would be lost. in fact, it would be lost even
when no subsequent GTE ever comes along to inspire the KJSM to work again on
this same job and it's state.
what can we do to prevent?
##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/KafkaJobStatusMonitor.java:
##########
@@ -236,8 +238,9 @@ protected void
processMessage(DecodeableKafkaRecord<byte[],byte[]> message) {
* It fills missing fields in job status and also merge the fields with the
* existing job status in the state store. Merging is required because we
* do not want to lose the information sent by other GobblinTrackingEvents.
- * Returns false if adding this state transitions the job status of the job
to final, otherwise returns false.
- * It will also return false if the job status was already final before
calling this method.
+ * Returns an absent Optional if adding this state transitions the job
status of the job to final, otherwise returns
+ * the updated job status wrapped inside an Optional.
+ * It will also return an absent Optional if the job status was already
final before calling this method.
Review Comment:
"exactly-once guarantee" might be clearest way to phrase.
with not choosing "at-least-once", we must consider what happens if this
method returns (and did successfully update the JobStateStore), but we fail
before completing the update to the DagActionStore.
seems the reevaluate action would be lost. in fact, it would be lost even
when no subsequent GTE ever comes along to inspire the KJSM to work again on
this same job and its state.
what can we do to prevent?
--
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]
