-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat:(restart-inbound-connectors): create a restart inbound mechanism after cancelling #3505
base: main
Are you sure you want to change the base?
Conversation
… after cancelling
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here depends of what we want to do:
- In this case,
retryAttempts
means 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we report the exception somewhere that cause the cancellation? Would be enough if its part of the health object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true 👍
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 milestone
label.