JAVA-3485: Suppress firing of duplicate cluster events #566
Conversation
The monitoring specification requires that duplicate server events are not sent. This refactoring separates server listeners registered by the application, to which this rule applies, from the one registered by Cluster implementation, to which this rule does not apply. JAVA-3485
Don't fire server or cluster description changed events when the description hasn't changed from the previously fired event. JAVA-3485
driver-core/src/main/com/mongodb/internal/connection/EventHelper.java
Outdated
Show resolved
Hide resolved
|
LGTM |
driver-core/src/main/com/mongodb/internal/connection/EventHelper.java
Outdated
Show resolved
Hide resolved
driver-core/src/test/unit/com/mongodb/internal/connection/EventHelperTest.java
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/connection/DefaultServer.java
Outdated
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/event/EventListenerHelper.java
Show resolved
Hide resolved
| return true; | ||
| } | ||
|
|
||
| static boolean equivalentEvent(final ServerDescription current, final ServerDescription previous) { |
There was a problem hiding this comment.
nit: Naming not comparing events, so the function name creates some unnecessary cognitive load.
There was a problem hiding this comment.
Yeah, the naming choice was deliberate, but I see why it's confusing. I didn't want to call it equivalentDescriptions, since that would beg the question of why we're not just using ServerDescription#equals. It's really more like 'wouldDescriptionsGenerateEquivalentEvents'. Would that be better? Or perhaps just a comment explaining it? Both?
There was a problem hiding this comment.
I think both would be good - it looks like a typo now and future Ross would probably wonder why we're not using equals!
| if (current.getServerDescriptions().size() != previous.getServerDescriptions().size()) { | ||
| return false; | ||
| } | ||
| for (ServerDescription curNew: current.getServerDescriptions()) { |
There was a problem hiding this comment.
Observation: I had a look at doing this with streams, to see if there was any nice patterns to for this / if it would be easier to read:
return !current.getServerDescriptions().stream().anyMatch(c ->
!previous.getServerDescriptions().stream()
.filter(p -> p.getAddress().equals(c.getAddress()))
.findFirst()
.map(p -> equivalentEvent(c, p))
.orElse(false)
);
I suppose it depends on preference as to which is more readable and I have no idea about comparative performance.
There was a problem hiding this comment.
I find it more natural with the loop, which might just be my lack of familiarity with streams. OK to keep it as is?
There was a problem hiding this comment.
Yeap - that was just for my learning
|
|
||
| import com.mongodb.event.ServerDescriptionChangedEvent; | ||
|
|
||
| // internal interface that Cluster implementations register with Server implementations in order be notified of changes in server state. |
There was a problem hiding this comment.
Nit: Use java doc / block comment pattern - even if javadocs arent produced for internal packages it keeps things normalized.
There was a problem hiding this comment.
Ah, I was using this style to make it less confusing, so readers wouldn't think the interface was public.
Done
|
Merged on command line |
The design problem presented by this requirement is that currently we combine the ServerListener registered by Cluster implementations to be informed of server changes with any ServerListeners registered by an application. This turns out to be incorrect, because different rules apply. The Cluster needs to be informed of all changes to server descriptions, e.g. including changes to RTT. But the spec requires that server events are only delivered to applications under certain rules of equality. So to implement this, in the first commit I refactored Server and Cluster implementations to separate the two types of listeners (adding a new private interface for the former). In the second commit I implemented the event suppression rules required by the specification.
https://spruce.mongodb.com/version/5f4ab0a92a60ed0c9dcaac25/tasks