Skip to content

Ensure serverHeartbeatEvent is sent before opening a connection #1715

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 4 commits into from
May 28, 2025

Conversation

rozza
Copy link
Member

@rozza rozza commented May 22, 2025

Added Heartbeat prose test
Migrated DefaultServerMonitorSpecification to JUnit 5.

JAVA-5230

Added Heartbeat prose test
Migrated DefaultServerMonitorSpecification to JUnit 5.

JAVA-5230
@rozza rozza requested review from Copilot, a team and katcharov and removed request for a team May 22, 2025 09:18
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates the legacy Groovy DefaultServerMonitorSpecification to JUnit 5 tests and refines the heartbeat-started event emission in DefaultServerMonitor.

  • Introduced DefaultServerMonitorTest.java with new JUnit 5 tests covering success, failure, and ordering of heartbeat events before socket connect.
  • Removed the old DefaultServerMonitorSpecification.groovy.
  • Updated DefaultServerMonitor.java to add a VisibleForTesting accessor and de-duplicate serverHeartbeatStarted events using a new flag.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerMonitorTest.java Added JUnit 5 tests for heartbeat events and ordering before connection open
driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerMonitorSpecification.groovy Removed legacy Groovy-based spec after migrating tests to JUnit 5
driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java Added getServerMonitor() for testing, alreadyLoggedHeartBeatStarted flag, and refined heartbeat-started logic
Comments suppressed due to low confidence (2)

driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java:161

  • [nitpick] Consider renaming 'alreadyLoggedHeartBeatStarted' to 'alreadyLoggedHeartbeatStarted' to follow consistent camelCase for the term 'heartbeat'.
private volatile boolean alreadyLoggedHeartBeatStarted = false;

driver-core/src/main/com/mongodb/internal/connection/DefaultServerMonitor.java:224

  • Add a unit test that simulates multiple heartbeat cycles to verify the 'alreadyLoggedHeartBeatStarted' flag behavior and ensure only one 'ServerHeartbeatStartedEvent' is emitted per cycle.
boolean shouldStreamResponses = shouldStreamResponses(currentServerDescription);

@rozza rozza removed the request for review from katcharov May 22, 2025 10:56
@rozza rozza marked this pull request as draft May 22, 2025 10:56
@rozza rozza requested a review from katcharov May 22, 2025 12:23
@rozza rozza marked this pull request as ready for review May 22, 2025 12:23
@rozza rozza requested a review from katcharov May 27, 2025 11:52
Copy link
Collaborator

@katcharov katcharov left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -166,7 +166,7 @@ public void serverDescriptionChanged(final ServerDescriptionChangedEvent event)
* <a href="https://github.com/mongodb/specifications/blob/master/source/server-discovery-and-monitoring/server-discovery-and-monitoring-tests.md#connection-pool-management">Connection Pool Management</a>.
*/
@Test
@Ignore
@Ignore("JAVA-4484 - events are not guaranteed to be delivered in order")
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@rozza rozza merged commit 25c0262 into mongodb:main May 28, 2025
54 checks passed
@rozza rozza deleted the JAVA-5230 branch May 28, 2025 07:59
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