-
Notifications
You must be signed in to change notification settings - Fork 988
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
Extending recording/publishing infra #2859
base: main
Are you sure you want to change the base?
Conversation
* Add publish interface in EventRecorder * Allow DefaultEventBus to publish event through EventRecorder * Wrap source Event in JfrRecordableEvent
void record(); | ||
|
||
} | ||
void publish(Event event); |
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 semantic difference between record(Event)
and publish(Event)
?
Such an arrangement asks for trouble because the methods are ambiguous without clear distinction.
RecordableEvent
is part of the API, moving it around introduces a breaking change without good reason. All necessary changes are possible without breaking the public API.
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 can move RecordableEvent
back to EventRecorder
.
I added publish
per #2819 (comment), specifically:
- Adopt
JfrEventRecorder.publish(…)
to check whether the given event isEventRecorder.RecordableEvent
. If so, then we callrecord(…)
on the event instead ofjdk.jfr.Event jfrEvent = createEvent(…);
Agreed that they create ambiguity. Should I combine the implementation of JfrEventRecorder.#record
and JfrEventRecorder#publish
into #record
?
@mp911de posted a new commits to address your last comment. Would you please take a look? |
Seems I messed up the previous PR a bit. Had to create a new one and carry the context here.
This PR is to fulfill this #2809 (comment) and address comments from https://github.com/redis/lettuce/pull/2819
This is more about getting the direction correct. Will supply more testing if the infra change looks good.
publish
interface inEventRecorder
DefaultEventBus
to publish event throughEventRecorder
RecordableEvent
Make sure that: