Temporal client method to restart failed workflow #15051
Conversation
github-actions
bot
added
area/platform
| @@ -446,6 +451,79 @@ public ManualOperationResult synchronousResetConnection(final UUID connectionId, | |||
| Optional.of(resetJobId), Optional.empty()); | |||
| } | |||
|
|
|||
| public void restartWorkflowByStatus(final WorkflowExecutionStatus executionStatus) { | |||
There was a problem hiding this comment.
👍 on executionStatus being an argument
| UUID.fromString(stringUUID)); | ||
| } | ||
|
|
||
| Set<String> fetchWorkflowByStatus(final WorkflowExecutionStatus executionStatus) { |
There was a problem hiding this comment.
| Set<String> fetchWorkflowByStatus(final WorkflowExecutionStatus executionStatus) { | |
| Set<String> fetchWorkflowsByStatus(final WorkflowExecutionStatus executionStatus) { |
This is a plural method, that returns a set
| final Set<String> workflowExecutionInfos = fetchWorkflowByStatus(executionStatus); | ||
|
|
||
| final Set<String> nonRunningWorkflow = filterOutRunningWorkspaceId(workflowExecutionInfos); |
There was a problem hiding this comment.
How large can these sets be? Are we worried about running out of memory? Should we be iterating in batches?
Maybe the method signature could be fetchWorkflowByStatus(executionStatus, limit=100) and we keep calling it until we get 0 results. In the method below, you are already paging though results with nextPagetoken - perhaps we don't need to do that.
There was a problem hiding this comment.
The limiting number here is number of connection ID. I don't expect it to be a concern in the short term.
In order to limit the memory consumption, I will change this to use UUID instead of String.
There was a problem hiding this comment.
I'm still concerned about this method returning quite a lot of data once we have 1000s of workspaces and connections, but I agree for now this is probably OK.
| + "unreachable state before starting a new workflow for this connection"); | ||
| ConnectionManagerUtils.startConnectionManagerNoSignal(client, maybeConnectionId.get()); | ||
| } else { | ||
| log.error("Malformated workflow id, won't terminate of restart"); |
There was a problem hiding this comment.
Gramatical Nit:
| log.error("Malformated workflow id, won't terminate of restart"); | |
| log.error("Malformated workflow id, won't terminate or restart"); |
| System.err.println(workflowId); | ||
| System.err.println(StringUtils.removeStart(workflowId, "connection_manager_")); | ||
| System.err.flush(); |
There was a problem hiding this comment.
We use log() elsewhere... can the logger not accept a log-level to print to stdout?
There was a problem hiding this comment.
Sorry for the miss-understanding. This is a draft and is not meant to be merge, so it contains debug print. This will be clean up. The main thing to look at is the test and the static way the mock are done.
…poral-client-restart-by-status
…poral-client-restart-by-status
…poral-client-restart-by-status
…poral-client-restart-by-status
benmoriceau
changed the title
There was a problem hiding this comment.
When trying to understand the scope of the ConnectionManagerUtils, it feels like we should look a bit closer at the scope of the ConnectorManagerWorkflow and the WorkflowClient. It is not super clear what's the responsibility of each object, this will be helpful in order to get a cleaner dependency injection.
…poral-client-restart-by-status
What
Described in #14970
Add a method in
TemporalClientthat will allow to restart all the failed temporal workflow.It also make
ConnectionManagerUtilsto be injectable (i.e. remove the statics).