phet commented on code in PR #3830:
URL: https://github.com/apache/gobblin/pull/3830#discussion_r1396577229
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/restli/GobblinServiceFlowExecutionResourceHandlerWithWarmStandby.java:
##########
@@ -46,56 +48,38 @@ public
GobblinServiceFlowExecutionResourceHandlerWithWarmStandby(FlowExecutionRe
this.dagActionStore = dagActionStore;
}
+ @Override
+ public void resume(ComplexResourceKey<FlowStatusId, EmptyRecord> key) {
+ FlowStatusId id = key.getKey();
+ addDagAction(id.getFlowGroup(), id.getFlowName(), id.getFlowExecutionId(),
DagActionStore.FlowActionType.RESUME);
+ }
Review Comment:
I'm not familiar enough w/ restli to know whether actions like resume can
actually return a result the way HTTP methods like DELETE (kill) do. I see
that [the
`FlowExecutionResource`](https://github.com/apache/gobblin/blob/a70f53913bb587da3471f946b1518c5c34b7f099/gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowExecutionResource.java#L43)
at least is written for the handlers to behave in this way. that said, when I
use curl to do a resume, I do observe a 200 OK coming back at me. perhaps
that's what's sent if a void method doesn't throw an exception. strictly
speaking, in this case where we queue the work, it should probably be HTTP 202
Accepted.
as for no 404, upon an entity not found, I intuitively agree w/ those
semantics and can't think of a counter-argument. do we think it was merely
forgotten? if so, I'm agree on adding, but suggest we do that on a separate
commit, since this is just a quick bug fix
--
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]
