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

Provide a way to listen to the events of mirroring #1057

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Nov 11, 2024

Motivation:

Mirroring failure is only recorded as a warning, but there is a need to handle it in a different way. For example, it can be recorded in metrics or end-users can be notified immediately.

MirrorListener is provided as an extension point to utilize various events occurring in the mirror.

Modifications:

  • Introduce MirrorListener whose implementations can be loaded dynamically via Java SPI.
  • onStart(), onComplete() and onError() events are added.
  • The default behavior is preserved in DefaultMirrorListener which is only used when no custom MirrorListener is configured.

Result:

You can now use MirrorListener to listen to Mirror events.

Motivation:

Mirroring failure is only recorded as a warning, but there is a need to
handle it in a different way. For example, it can be recorded in metrics
or end-users can be notified immediately.

`MirrorListener` is provided as an extension point to utilize various
events occurring in the mirror.

Modifications:

- Introduce `MirrorListener` whose implementations can be loaded dynamically
  via Java SPI.
- `onStart()`, `onComplete()` and `onError()` events are added.
- The default behavior is preserved in `DefaultMirrorListener` which is
  only used when no custom `MirrorListener` is configured.

Result:

You can now use `MirrorListener` to listen to `Mirror` events.
@ikhoon ikhoon added this to the 0.71.0 milestone Nov 11, 2024
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Looks great! 👍 👍 👍

/**
* Invoked when the {@link Mirror} operation is failed.
*/
void onError(Mirror mirror, Throwable cause);
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional; Personally, I feel like MirrorResult could be useful for onError as well since it contains information on mirrorId, triggeredTime, etc....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed to pass MirrorTask which contains those properties.

@@ -62,6 +66,19 @@ public final class MirrorSchedulingService implements MirroringService {

private static final Logger logger = LoggerFactory.getLogger(MirrorSchedulingService.class);

private static final MirrorListener mirrorListener;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note) I understood that MirrorListener isn't invoked for manually triggered mirrors from MirrorServiceV1.

While this may make sense in terms of our purpose (monitoring), it may be confusing since a Mirror has been done yet MirrorListener isn't invoked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Let me scheduled flag to methods of MirrorListener.

/**
* Invoked when the {@link Mirror} operation is started.
*/
void onStart(Mirror mirror);
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional; If a MirrorTask is passed at onStart which contains a mirrorId, users may be able to differentiate the flow of a single Mirror across multiple callbacks via mirrorId. This may be useful since multiple mirrors may be run in a single leader with the introduction of zone-aware mirroring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mirrorId means the ID for a single mirroring task, not Mirror#id(), right?

This may be useful since multiple mirrors may be run in a single leader with the introduction of zone-aware mirroring.

Sorry, I didn't understand this. Did you mean multiple mirrors can be run concurrently? Could you elaborate on this, please?

@ikhoon ikhoon merged commit bed87a2 into line:main Nov 12, 2024
10 checks passed
@ikhoon ikhoon deleted the mirror-listener branch November 12, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants