Skip to content

Ensure server discovery and monitoring events are published in order #860

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

Merged
merged 6 commits into from
Jan 28, 2022

Conversation

jyemin
Copy link
Collaborator

@jyemin jyemin commented Jan 25, 2022

  • Ensure events are ordered according to how they are applied to driver state
  • Publish all events to application listeners one at a time, on a separate thread

JAVA-4449

* Ensure events are ordered according to how they are applied to driver state
* Publish all events to application listeners one at a time, on a separate thread

JAVA-4449
Copy link
Collaborator Author

@jyemin jyemin left a comment

Choose a reason for hiding this comment

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

Simplified based on in-person conversation. PTAL.

@stIncMale
Copy link
Member

I like the new approach for decorating listeners 👍

[optional]
I propose to assert that there is exactly 1 listener in the constructor of BaseCluster and then just use it, instead of calling getClusterListener(settings). We know that it must be the case for all clusters and servers because of how DefaultClusterFactory works.

If you agree, the same can be done also in

  • LoadBalancedCluster
  • DefaultClusterableServerFactory
  • LoadBalancedClusterableServerFactory
  • DefaultServerMonitor

As a result, the only place where EventListenerHelper.getClusterListener/getServerListener/getServerMonitorListener are called will be DefaultClusterFactory. Additionally, we could even move the methods there and make them private to avoid using them accidentally in a new server/cluster/factory implementation.

@jyemin jyemin requested a review from stIncMale January 28, 2022 00:05
ClusterSettings clusterSettings;
ServerSettings serverSettings;

if (noClusterEventListeners(originalClusterSettings, originalServerSettings)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since the vast number of clients never register a listener, avoiding thread/queue creation for that case.

&& serverSettings.getServerMonitorListeners().isEmpty();
}

private static ClusterListener getClusterListener(final ClusterSettings clusterSettings) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Small behavioral change here. Using multicaster when there is one listener registered, because the muilticaster will log exceptions thrown by the listeners, which should happen in all cases. Didn't seem worth it to add a non-multicasting exception-logging implementation.

@jyemin jyemin merged commit f6b540c into mongodb:master Jan 28, 2022
@jyemin jyemin deleted the j4449 branch January 28, 2022 22:52
jyemin added a commit that referenced this pull request Feb 7, 2022
…860)

* Ensure events are ordered according to how they are applied to driver state
* Publish all events to application listeners one at a time, on a separate thread
* Document and ensure that cluster event listeners are not required to be thread safe

JAVA-4449
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.

2 participants