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

Initial commit for dual publishing TopologyRefreshEvent to EventBus #2819

Closed
wants to merge 1 commit into from

Conversation

rkeely
Copy link

@rkeely rkeely commented Apr 8, 2024

Publish TopologyRefreshEvent to event bus #2809

Update: 2024-04-07

Only include interface change to get a sense of direction before larger change.

Some reasoning:

  1. Creating a new interface called DurationalEvent to capture the traits of event thats not ephemeral or instantaneous.
    I didn't reuse the Event interface because most of events that implements it are instantaneous and I don't want them to
    carry traits that could indicate anything that's related to duration.
  2. Adding new methods in EventRecorder to explicitly handle dual publishing (recording with recorder and publishing with event bus or only recording). This seems to be the most straight forward and I am also open to creating a new wrapper class for dual publishing only.
  3. Reusing TopologyRefreshEvent as I suspect there are many people subscribing to it from EventBus already. I am also open to create new events class dedicated for the start and end for topology refresh operation.

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

Only include interface change to get a sense.
@rkeely rkeely marked this pull request as draft April 8, 2024 03:05
@rkeely rkeely marked this pull request as ready for review April 8, 2024 03:08
@mp911de
Copy link
Collaborator

mp911de commented Apr 9, 2024

Extending individual events requires duplicating a lot of the functionality within the individual events and it makes these mutable.

I was looking more for an approach where we extend the recording/publishing infrastructure. In particular:

  • Add Event getSource() to RecordableEvent and make RecordableEvent extends Event
  • Capture the original event in JfrRecordableEvent and rework NoOpEventRecorder to create an event instance
  • Long-lived events would be published to EventBus (e.g. EventRecorder.RecordableEvent event = …; later: eventBus.publish(event).
  • Adopt JfrEventRecorder.publish(…) to check whether the given event is EventRecorder.RecordableEvent. If so, then we call record(…) on the event instead of jdk.jfr.Event jfrEvent = createEvent(…);
  • Retrofit DefaultEventBus to unwrap EventRecorder.RecordableEvent before emitting the event

@mp911de mp911de added the type: enhancement A general enhancement label Apr 9, 2024
@rkeely rkeely closed this May 20, 2024
@rkeely rkeely deleted the topology-refresh-on-bus branch May 20, 2024 05:10
@rkeely
Copy link
Author

rkeely commented May 20, 2024

Sorry that I deleted my PR branch while doing some rebase.
I have created a new PR #2859. Let's continue the discussion in the new one

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

Successfully merging this pull request may close these issues.

None yet

2 participants