feat:(restart-inbound-connectors): create a restart inbound mechanism after cancelling #3505
Conversation
… after cancelling
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
connector-sdk/core/src/main/java/io/camunda/connector/api/error/RestartException.java
Fixed
Show resolved
Hide resolved
| executables.remove(uuid); | ||
| executables.put( | ||
| uuid, new RegisteredExecutable.Cancelled(activated.executable(), activated.context())); |
There was a problem hiding this comment.
Could you use Map.replace() to switch the entry?
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class CancellationManager { |
There was a problem hiding this comment.
Can you provide some documentation what the ConcellationManager is responsible for?
| executables.remove(uuid); | ||
| executables.put( | ||
| uuid, new RegisteredExecutable.Activated(cancelled.executable(), cancelled.context())); |
There was a problem hiding this comment.
Could you use Map.replace() to switch the entry?
|
|
||
| private void tryRestart( | ||
| UUID uuid, RegisteredExecutable toReactivate, Duration delay, Integer retryAttempts) { | ||
| if (retryAttempts < 0) return; |
There was a problem hiding this comment.
What is the specification here? We stop restart if the the attempts are for example -1? Its not clear when this would happen
There was a problem hiding this comment.
Here depends of what we want to do:
- In this case,
retryAttemptsmeans that if the user has set 3 retries, It means it will initially try then retry 3 times - In the other case, if the user has set 3 retries, it will try initially and then try again 2 times
What would a developer expect ?
There was a problem hiding this comment.
I am not sure but retryAttempts sounds like the number of attempts that already happened. I would prefer a sealed interface with first of two options: Just a restart and one with a delay.
Lets tackle the number of retries later.
| case Cancelled cancelled -> | ||
| new ActiveExecutableResponse( | ||
| id, | ||
| cancelled.executable().getClass(), | ||
| cancelled.context().connectorElements(), | ||
| cancelled.context().getHealth(), | ||
| cancelled.context().getLogs()); |
There was a problem hiding this comment.
Do we report the exception somewhere that cause the cancellation? Would be enough if its part of the health object.
|
LGTM, some minor comments and please add some tests for the different paths. |
Description
Second try for implementing the restart mechanism, I have tested it and it seems to be working
Related issues
closes #
Checklist
no milestonelabel.