Add durations to connection pool events #1166

stIncMale · 2023-08-02T18:45:17Z

This PR implements the spec change mongodb/specifications#1448.

JAVA-5076

JAVA-5105

rozza

Couple of comments / questions and nits

driver-core/src/main/com/mongodb/event/ConnectionCheckOutFailedEvent.java

driver-core/src/main/com/mongodb/internal/connection/ConnectionPool.java

rozza · 2023-08-23T15:39:23Z

driver-core/src/main/com/mongodb/event/ConnectionCheckOutFailedEvent.java

@@ -77,6 +99,7 @@ public ConnectionCheckOutFailedEvent(final ServerId serverId, final long operati
     * @param serverId the server id
     * @param reason the reason the connection check out failed
     * @deprecated Prefer {@link #ConnectionCheckOutFailedEvent(ServerId, long, Reason)}
+     * If this constructor is used, then {@link #getOperationId()} is -1.


Suggestion: If expanding out the defaults in the java docs then also include:

and {@link #getElapsedTime(TimeUnit)} is 0.

(This applies to all deprecated constructors with a default operation id)

I deliberately did not specify anything about getElapsedTime when documenting this constructor. The idea is that when we deprecate a constructor and introduce a new one, we should only update the documentation for the deprecated constructor and write the documentation for the new one. The reason behind this approach is that we are not going to remember and update all previous constructors each time we introduce a new one, which will lead to weird inconsistent documentation.

When we say Prefer {@link ...(ServerId, long, Reason, long)}. If this constructor is used, then {@link #getElapsedTime(TimeUnit)} is 0., a reader sees that the only difference between the preferred constructor and the one he looks at is a single parameter elapsedTimeNanos, and the documentation explains what value the current constructor uses given that it does not accept elapsedTimeNanos from a user.

Similarly, when we say Prefer {@link ...(ServerId, long, Reason)}. If this constructor is used, then {@link #getOperationId()} is -1., a reader sees that the only difference between the preferred constructor and the one he looks at is a single parameter operationId, and the documentation explains what value the current constructor uses given that it does not accept operationId from a user.

The same goes about the preferred constructors. When we introduce a new constructor, the preferred one specified in all previously deprecated constructors is not replaced with the new constructor. It stays unchanged, thus forming a chain of preference, that leads a reader from any constructor to a more and more preferred one. The chain ends with the non-deprecated constructor.

Given this description of the approach and the reason behind it (both are in bold above), do you still want the docs for all previous constructors to be updated when a new one is introduced?

I can live with that 👍

driver-core/src/main/com/mongodb/internal/connection/DefaultConnectionPool.java

...core/src/test/unit/com/mongodb/internal/connection/DefaultConnectionPoolSpecification.groovy

JAVA-5076

rozza

LGTM

rozza · 2023-08-24T09:25:37Z

driver-core/src/main/com/mongodb/internal/connection/DefaultConnectionPool.java

-    private void connectionCheckoutStarted(final OperationContext operationContext) {
+
+    /**
+     * @return A {@link TimePoint} after executing {@link ConnectionPoolListener#connectionCheckOutStarted(ConnectionCheckOutStartedEvent)}.


katcharov

Code looks nice and straightforward, just doc clarifications.

driver-core/src/main/com/mongodb/connection/ConnectionPoolSettings.java

driver-core/src/main/com/mongodb/ConnectionString.java

driver-core/src/main/com/mongodb/connection/ConnectionPoolSettings.java

katcharov · 2023-08-24T19:20:02Z

driver-core/src/main/com/mongodb/event/ConnectionCheckOutFailedEvent.java

+     * This duration does not currently include the time to deliver the {@link ConnectionCheckOutStartedEvent}.
+     * Subject to change.</p>


getElapsedTime. Just curious, in the spec, what you said about user code (which I think referred to this) seemed right. What is the reason this might change?

🤷‍♂️ Maybe users will request for that, maybe something else I can't anticipate comes up. There is an opportunity to allow for a future change, and I prefer to take it.

When we say "Subject to change", it seems to negate the documented behaviour immediately preceding, since users cannot rely on it. This reminded me of @Beta. I also thought to search for the string "Subject to change" to see if we take this approach elsewhere, and the one result was:

APIs marked with the `@Beta` annotation at the class or method level are subject to change.

For any API we do not consider Beta, I think we should either avoid documenting anything that is subject to change (in cases where the external behaviour is trivial or irrelevant to users), or commit to a behaviour. My opinion is that committing is preferable, assuming it is reasonable for a user to ask "but does this include user code"?

"Subject to change", it seems to negate the documented behaviour immediately preceding, since users cannot rely on it.

We could say "we don't provide any guarantees on whether this duration includes the time to deliver ...". Instead, we give users more information: we document the current behavior (which enables a user to understand why his Thread.sleep(1_000) in a listener does not affect the duration reported in some events), but don't guarantee it will stay the same. Also, because the current behavior is documented, users know that if it is changed, the change will be visible in the documentation.

For any API we do not consider Beta, I think we should either avoid documenting anything that is subject to change

@Beta is about API changes, which is not what we are talking about here.

My opinion is that committing is preferable, assuming it is reasonable for a user to ask "but does this include user code"

This PR implements the spec requirement "The driver MUST document this behavior as well as explicitly warn users that the behavior may change in the future". Are you require the spec to be re-considered? If yes, is this concern a PR blocker? Note that if a DRIVERS ticket is created, and the spec is changed as a result, incorporating such a change in the Java driver will not break compatibility.

assuming it is reasonable for a user to ask "but does this include user code" - I don't understand this part. Could you explain it?

Described behaviour is rightly taken by users to be part of the public API. Some public behaviour might be tentative ("subject to change"), but my view is that all such instances in the driver should be marked with @Beta, and that this is done with the expectation that we think the present design is right, and will "soon" be removed from Beta. I do think it is a mistake for any non-beta spec to require drivers to mark public behaviour as tentative.

The original wording was describing public behaviour, however, I believe you are proposing that we might instead state this behaviour but clarify that it is non-public. I don't think we (or others) should describe implementation details, especially without a compelling reason. Otherwise, we might include many internal hints and tips in our docs.

So:

If public behaviour is "subject to change", it should be marked Beta, which is not applicable here.

Non-public behaviour is inherently "subject to change", but I think we should not mention internal behaviour.

My opinion is that you made the right call in excluding user code, and that we should publicly commit to that behaviour. I do not mind merging and changing this later via tickets, if that is more convenient.

Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>

JAVA-5076

katcharov

Resolved some, still discussing others.

JAVA-5076

katcharov

Approved, and created https://jira.mongodb.org/browse/DRIVERS-2707 for the minor spec change.

JAVA-5076

stIncMale self-assigned this Aug 2, 2023

stIncMale force-pushed the JAVA-5076 branch 4 times, most recently from dda0aea to ecdb92f Compare August 3, 2023 07:20

stIncMale marked this pull request as draft August 3, 2023 07:21

stIncMale force-pushed the JAVA-5076 branch 5 times, most recently from 351fbd4 to e3da902 Compare August 4, 2023 06:14

stIncMale mentioned this pull request Aug 4, 2023

Add TimePoint and use it in Timeout #1164

Merged

stIncMale force-pushed the JAVA-5076 branch from e3da902 to 2b96fec Compare August 4, 2023 15:51

stIncMale mentioned this pull request Aug 4, 2023

DRIVERS-2469 Add durations to connection pool events mongodb/specifications#1448

Merged

4 tasks

stIncMale force-pushed the JAVA-5076 branch from 2495362 to 954c6f4 Compare August 8, 2023 22:31

Add durations to connection pool events

bbee638

JAVA-5076

stIncMale force-pushed the JAVA-5076 branch from a2121a3 to bbee638 Compare August 21, 2023 21:46

stIncMale marked this pull request as ready for review August 21, 2023 22:27

stIncMale requested review from rozza and katcharov August 21, 2023 22:27

Specify the release version

9989a43

JAVA-5105

rozza requested changes Aug 23, 2023

View reviewed changes

Address review concerns

f2c77c1

JAVA-5076

stIncMale requested a review from rozza August 24, 2023 03:32

rozza approved these changes Aug 24, 2023

View reviewed changes

katcharov requested changes Aug 24, 2023

View reviewed changes

stIncMale and others added 2 commits August 24, 2023 14:22

Apply suggestions from code review

83a703b

Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>

Address review concerns

6f93086

JAVA-5076

stIncMale requested a review from katcharov August 24, 2023 20:49

katcharov requested changes Aug 24, 2023

View reviewed changes

Address review concerns

d5437f2

JAVA-5076

stIncMale requested a review from katcharov August 25, 2023 19:53

Fix grammar

8ca8ab2

JAVA-5076

katcharov approved these changes Aug 25, 2023

View reviewed changes

Fix grammar

981c91d

JAVA-5076

stIncMale merged commit 2fa9f49 into mongodb:master Aug 25, 2023

stIncMale deleted the JAVA-5076 branch August 25, 2023 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add durations to connection pool events #1166

Add durations to connection pool events #1166

stIncMale commented Aug 2, 2023 •

edited

Loading

rozza left a comment

rozza Aug 23, 2023

stIncMale Aug 24, 2023

rozza Aug 24, 2023 •

edited

Loading

rozza left a comment

rozza Aug 24, 2023

katcharov left a comment

katcharov Aug 24, 2023

stIncMale Aug 24, 2023

katcharov Aug 24, 2023

stIncMale Aug 24, 2023

katcharov Aug 25, 2023

katcharov left a comment

katcharov left a comment

		* This duration does not currently include the time to deliver the {@link ConnectionCheckOutStartedEvent}.
		* Subject to change.</p>

Add durations to connection pool events #1166

Add durations to connection pool events #1166

Conversation

stIncMale commented Aug 2, 2023 • edited Loading

rozza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rozza Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

rozza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katcharov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katcharov left a comment

Choose a reason for hiding this comment

katcharov left a comment

Choose a reason for hiding this comment

stIncMale commented Aug 2, 2023 •

edited

Loading

rozza Aug 24, 2023 •

edited

Loading