External Storage Integration: WorkflowWorker, replay, history - #3017
External Storage Integration: WorkflowWorker, replay, history#3017cconstable wants to merge 22 commits into
Conversation
dff9b7f to
3a20dcf
Compare
831c284 to
460bfbf
Compare
3a20dcf to
0e37f9d
Compare
cconstable
changed the title
0e37f9d to
c67e0f0
Compare
460bfbf to
b3804da
Compare
c67e0f0 to
e292757
Compare
b3804da to
ca09b50
Compare
e292757 to
cb05443
Compare
6d90f7e to
1538817
Compare
2d2d90e to
504467c
Compare
d7b9a62 to
8062db7
Compare
| } catch (CancellationException e) { | ||
| // if the worker is shutting down, extstore will throw a CancellationException and we need to | ||
| // rethrow it here so the handle() method can decide what to do. | ||
| throw e; |
There was a problem hiding this comment.
Rather than absorbing a cancellation exception it's just rethrown. I think this covers all the cases we want and it allows the caller (handle()) to determine whether or not to actually send a task failure to the server... if we are in the middle of a forced shutdown it will see this cancellation error and just absorb it instead of throwing (which would result in a task failure being emitted).
| || ex instanceof CancellationException | ||
| || ex.getCause() instanceof CancellationException |
There was a problem hiding this comment.
During shutdown we shouldn't emit task failures due to things being cancelled because of the shutdown. At the moment, I don't think this actually catches any external storage stuff (since this check happens at a higher level than where the external storage exceptions are caught) but I think it's the "correct" behavior to add regardless and would prevent future issues. Would like some feedback on this @Quinn-With-Two-Ns or @maciejdudko
There was a problem hiding this comment.
LGTM. Can adjust in future if need be.
| } catch (CancellationException e) { | ||
| if (!options.getStorageCancellation().isCancellationRequested()) { | ||
| throw e; | ||
| } | ||
| log.trace("Abandoned a workflow task while the worker was shutting down", e); | ||
| return; |
There was a problem hiding this comment.
When extstore throws a cancellation, if we are not forcefully shutting down then rethrow the error and report a failure. Otherwise, we should accept that the cancellation is just a normal part of the shutdown. The same logic was added below. It's possible that this cancellation exception might not originate from external storage in the future but I still think this is the correct course of action.
…nal payloads after fetching history.
…ng command.getAttributesCase() + switch for exhaustiveness checking.
…ellation gracefully during shutdown
…retarget to parent for storage target.
…ead of threading it through and cancel extstore AFTER the poller shutdown flag is set (was previously before).
|
Resolving conflicts... |
47a39ca to
3e2a17e
Compare
| workflowTypeScope, | ||
| workflowStorageTarget(workflowExecution, workflowType)); | ||
| } catch (ExternalStorageTaskFailure e) { | ||
| Failure failure = |
There was a problem hiding this comment.
Probably should use logAbandonedReport here.
There was a problem hiding this comment.
Well this is technically not abandoning the response because it calls sendDirectQueryCompletedResponseWithoutExternalStorage. I think that should work but I also think it's maybe a little inconsistent... sometimes we respond and sometimes we just log. Should we be sending a failure response when we can? In this case, I think it's safe to send but correct me if I'm wrong.
2 participants
What was changed
WorkflowWorkerhandle + send response now use external storage.Why
Checklist