Skip to content
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): give the possibility to the developer to plan a restart of the connector in case of failure #3486

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mathias-vandaele
Copy link
Contributor

Description

Proposal of solution for the restart of inbound connector, not tested yet

Related issues

closes #3319

Checklist

  • PR has a milestone or the no milestone label.

…a restart of the connector in case of failure
@mathias-vandaele mathias-vandaele requested a review from a team as a code owner October 15, 2024 17:12
@mathias-vandaele mathias-vandaele linked an issue Oct 15, 2024 that may be closed by this pull request
@sbuettner
Copy link
Contributor

sbuettner commented Oct 16, 2024

Hey @mathias-vandaele, the ticket contains an implementation suggestion: #3319 to re-use the existing cancellation mechanism. Would be interesting to understand why you see the need to introduce another method?

@mathias-vandaele
Copy link
Contributor Author

Hey @mathias-vandaele, the ticket contains an implementation suggestion: #3319 to re-use the existing cancellation mechanism. Would be interesting to understand why you see the need to introduce another method?

@sbuettner everything still happens during the cancellation mechanism

@Override
 public void cancel(Throwable exception) {
   try {
     cancellationCallback.accept(exception);
   } catch (Throwable e) {
     LOG.error("Failed to deliver the cancellation signal to the runtime", e);
   }
   if (Objects.requireNonNull(exception) instanceof RestartException restartException) {
     reactivationCallback.accept(restartException.getBackoffTime());
   }
 }

A callback is declared in order to be executed after cancelation 👍


import java.time.Duration;

public class RestartException extends RuntimeException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to reuse the ConnectorRetryException with the retries and backoff parameters?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnBgood Good idea although we would need a way to restart without defining the number of retries. We could add it. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, either that or we add the retries count to the context and when throwing the exception later we use this number, similar to how it's done for Outbound connectors.

@sbuettner
Copy link
Contributor

@mathias-vandaele Ah, I was confused as I saw: reactivationCallback. Would it be simpler to re-use the existing cancellation callback and handle it in the consumer?

"Inbound connector executable not found for reactivation, received ID: {}", id);
return;
}
if (toReactivate instanceof Activated activated) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it an issue if a Connector had an error? How do we handle multiple activation failure in this case?

private final BlockingQueue<InboundExecutableEvent> eventQueue;
private final ScheduledExecutorService reactivationScheduler =
Executors.newSingleThreadScheduledExecutor();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to use a single thread executor here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support inbound Connector restarts
3 participants