phet commented on code in PR #3700:
URL: https://github.com/apache/gobblin/pull/3700#discussion_r1227765802
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/FlowTriggerHandler.java:
##########
@@ -96,82 +93,79 @@ public SchedulerLeaseAlgoHandler(Config config,
MultiActiveLeaseArbiter leaseDet
* @param eventTimeMillis
* @throws IOException
*/
- public void handleNewSchedulerEvent(Properties jobProps,
DagActionStore.DagAction flowAction, long eventTimeMillis)
+ public void handleTriggerEvent(Properties jobProps, DagActionStore.DagAction
flowAction, long eventTimeMillis)
throws IOException {
- LeaseAttemptStatus leaseAttemptStatus =
+ MultiActiveLeaseArbiter.LeaseAttemptStatus leaseAttemptStatus =
multiActiveLeaseArbiter.tryAcquireLease(flowAction, eventTimeMillis);
// TODO: add a log event or metric for each of these cases
- switch (leaseAttemptStatus.getClass().getSimpleName()) {
- case "LeaseObtainedStatus":
- finalizeLease((LeaseObtainedStatus) leaseAttemptStatus, flowAction);
- break;
- case "LeasedToAnotherStatus":
- scheduleReminderForEvent(jobProps, (LeasedToAnotherStatus)
leaseAttemptStatus, flowAction, eventTimeMillis);
- break;
- case "NoLongerLeasingStatus":
- break;
- default:
+ if (leaseAttemptStatus instanceof
MultiActiveLeaseArbiter.LeaseObtainedStatus) {
+ persistFlowAction((MultiActiveLeaseArbiter.LeaseObtainedStatus)
leaseAttemptStatus, flowAction);
+ return;
+ } else if (leaseAttemptStatus instanceof
MultiActiveLeaseArbiter.LeasedToAnotherStatus) {
+ scheduleReminderForEvent(jobProps,
(MultiActiveLeaseArbiter.LeasedToAnotherStatus) leaseAttemptStatus, flowAction,
+ eventTimeMillis);
+ } else if (leaseAttemptStatus instanceof
MultiActiveLeaseArbiter.NoLongerLeasingStatus) {
+ return;
}
+ log.warn("Received type of leaseAttemptStatus: {} not handled by this
method", leaseAttemptStatus.getClass().getName());
Review Comment:
since this represents a gaping hole in our impl, it's actually more
appropriate to scream/panic/freak out via `RuntimeException`, than it is to
presume to continue (as if completely shirking all responsibility for the
`flowAction`)
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/FlowTriggerHandler.java:
##########
@@ -96,82 +93,79 @@ public SchedulerLeaseAlgoHandler(Config config,
MultiActiveLeaseArbiter leaseDet
* @param eventTimeMillis
* @throws IOException
*/
- public void handleNewSchedulerEvent(Properties jobProps,
DagActionStore.DagAction flowAction, long eventTimeMillis)
+ public void handleTriggerEvent(Properties jobProps, DagActionStore.DagAction
flowAction, long eventTimeMillis)
throws IOException {
- LeaseAttemptStatus leaseAttemptStatus =
+ MultiActiveLeaseArbiter.LeaseAttemptStatus leaseAttemptStatus =
multiActiveLeaseArbiter.tryAcquireLease(flowAction, eventTimeMillis);
// TODO: add a log event or metric for each of these cases
- switch (leaseAttemptStatus.getClass().getSimpleName()) {
- case "LeaseObtainedStatus":
- finalizeLease((LeaseObtainedStatus) leaseAttemptStatus, flowAction);
- break;
- case "LeasedToAnotherStatus":
- scheduleReminderForEvent(jobProps, (LeasedToAnotherStatus)
leaseAttemptStatus, flowAction, eventTimeMillis);
- break;
- case "NoLongerLeasingStatus":
- break;
- default:
+ if (leaseAttemptStatus instanceof
MultiActiveLeaseArbiter.LeaseObtainedStatus) {
+ persistFlowAction((MultiActiveLeaseArbiter.LeaseObtainedStatus)
leaseAttemptStatus, flowAction);
+ return;
+ } else if (leaseAttemptStatus instanceof
MultiActiveLeaseArbiter.LeasedToAnotherStatus) {
+ scheduleReminderForEvent(jobProps,
(MultiActiveLeaseArbiter.LeasedToAnotherStatus) leaseAttemptStatus, flowAction,
+ eventTimeMillis);
+ } else if (leaseAttemptStatus instanceof
MultiActiveLeaseArbiter.NoLongerLeasingStatus) {
+ return;
}
+ log.warn("Received type of leaseAttemptStatus: {} not handled by this
method", leaseAttemptStatus.getClass().getName());
Review Comment:
since this represents a gaping hole in our impl, it's actually more
appropriate to scream/panic/freak out via `RuntimeException`, than to presume
to continue (as if completely shirking all responsibility for the `flowAction`)
--
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]
