-
Notifications
You must be signed in to change notification settings - Fork 2
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
Event Bus in Native Java #324
Conversation
e49aaa4
to
968aa47
Compare
c957b00
to
082e09f
Compare
082e09f
to
2c3a4ca
Compare
6ee52a1
to
7916673
Compare
7916673
to
1c682f3
Compare
2930b70
to
a316ac0
Compare
a316ac0
to
98841fd
Compare
plugin/src/software/aws/toolkits/eclipse/amazonq/broker/EventBroker.java
Show resolved
Hide resolved
plugin/src/software/aws/toolkits/eclipse/amazonq/events/TestEvent.java
Outdated
Show resolved
Hide resolved
|
||
@SuppressWarnings("unchecked") | ||
default Class<T> getEventType() { | ||
Class<?> currentClass = getClass(); |
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.
Are there any references that you took inspiration from to compose this class? If yes, those would be useful to link in the description here
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.
Yeah I used stack overflow for this, but it wasn't exactly this. I can try to find it. It think it was this one: https://stackoverflow.com/questions/3437897/how-do-i-get-a-class-instance-of-generic-type-t. I'll add it to the code.
|
||
@Override | ||
@SuppressWarnings("unchecked") | ||
default Class<T> getEventType() { |
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 difference between StreamObserver and EventObserver? I see a few new functions in this class but the getEventType looks the same
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.
The EventObserver is the base bones version of a subscriber that only cares about events on the data channel. The StreamObserver extends this functionality with hooks for when the stream errors out or completes. The interfaces were introduced because the standard subscriber method onNext
wasn't descriptive or I felt like it didn't communicate that we were listening to/handling events. Also we'd be forced to also implement the onError
and onComplete
methods everytime.
@@ -44,6 +50,16 @@ public Activator() { | |||
.initializeOnStartUp() | |||
.build(); | |||
codeReferenceLoggingService = DefaultCodeReferenceLoggingService.getInstance(); | |||
|
|||
List<TestSubscribers> testSubscriberList = new ArrayList<>(3); |
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 expect Activator to be the source of truth for event broker subscriptions?
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.
No, this was just to test that the event bus is working. Subscriptions can come from anywhere in the codebase.
public void onEvent(final TestEvent event) { | ||
// Activator.getLogger().info(event.getMessage()); | ||
|
||
if (event.getSequenceNumber() - previousSequenceNumber != 1) { |
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 unclear on the significance of the sequence number here
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.
The sequence numbers are more for testing the POC on my end. They helped ensure that the events arrived in the order in which they were published to all subscribers.
AuthStatusProvider.addAuthStatusChangeListener(amazonQCommonActions.getFeedbackDialogContributionAction()); | ||
AuthStatusProvider.addAuthStatusChangeListener(amazonQCommonActions.getCustomizationDialogContributionAction()); | ||
authStateSubscription = EventBroker.getInstance().subscribe(this); | ||
EventBroker.getInstance().subscribe(amazonQCommonActions.getSignoutAction()); |
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.
Cancels are missing for l88-90 right?
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.
Yup, but they were not implemented in the original code either. To provide cancel functionality, we'd just need to keep track of the Subscription returned.
public final class TestPublisher { | ||
|
||
public TestPublisher() { | ||
Thread publisherThread = new Thread(() -> { |
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 we not map this to the auth events that get posted?
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.
We have separate publishers for auth events. This was just to demo that the event bus was performant and could deliver events in the order they were posted.
} | ||
|
||
public static void notifyAuthStatusChanged(final AuthState authState) { | ||
if (prevAuthState != null && prevAuthState.equals(authState)) { |
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 think this check is missing from the publisher calls when an event is published to the broker
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.
No, I added de-duplication logic to the event broker itself here.
Issue #322
Description of changes:
This PR introduces an Event Bus implementation utilizing the Java 9 Reactive Flow library. The event bus extends the functionality provided by the Flow API by adding custom concurrency constructs to enable asynchronous publishing and subscribing.
Brief Design Overview:
This implementation builds upon the basic publisher provided by the Flow API library, introducing a two-tier concurrency model to optimize performance and responsiveness. Here's a breakdown of the solution:
Publisher Side:
Subscriber Side:
In essence, this solution creates a pipeline where a dedicated thread handles event publishing for each event type, while on the subscriber side, each subscriber's callbacks are processed by a single thread, regardless of the number of events. By maintaining an event queue, we can guarantee event ordering for each subscriber.
Key features:
Current limitations and areas for further investigation:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.