Skip to content

Conversation

p-mongo
Copy link
Contributor

@p-mongo p-mongo commented Oct 14, 2019

No description provided.

@p-mongo p-mongo changed the title SPEC-1396 Drivers MUST clear connection pools when SDAM monitoring fails due to a network error SPEC-1396 Drivers MUST clear connection pools when SDAM monitoring fails due to a network error Oct 14, 2019
@p-mongo p-mongo requested review from mbroadst and jyemin October 14, 2019 23:36
@jyemin jyemin requested a review from ShaneHarvey October 14, 2019 23:51
When a server `check`_ fails due to a network error,
the client SHOULD clear its connection pool for the server:
When a server `check`_ fails due to a network error (including a timeout),
the client MUST clear its connection pool for the server:
Copy link
Member

Choose a reason for hiding this comment

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

Any ideas of how we can test this? Maybe in the CMAP spec tests we can configure isMaster to fail once with a network error via configureFailPoint, wait for the monitor to run the doomed isMaster, and then assert that the pool has been cleared.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not saying we need tests in this PR. Just spitballing. The current changes LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote several tests in mongodb/mongo-ruby-driver#1475 but I would prefer to wait until the unified spec runner work is complete before proposing spec tests that touch both cmap and sdam.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to wait for that? It's a non-goal in the unified spec runner scope to define the format for non-”operations” tests, including CMAP and SDAM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our cmap spec runner does not do any i/o, thus as it is implemented it is unable to set fail points on the server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ShaneHarvey It appears that failcommand fail point is ignored for ismaster commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out it does work when ismaster is given in mixed case, https://jira.mongodb.org/browse/SERVER-44414.

When a server `check`_ fails due to a network error,
the client SHOULD clear its connection pool for the server:
When a server `check`_ fails due to a network error (including a timeout),
the client MUST clear its connection pool for the server:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not saying we need tests in this PR. Just spitballing. The current changes LGTM.


When a server `check`_ fails due to a network error,
the client SHOULD clear its connection pool for the server:
When a server `check`_ fails due to a network error (including a timeout),
Copy link
Member

Choose a reason for hiding this comment

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

The spec defines terms "network error" and "network timeout", should that second term be used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

if the monitor's socket is bad it is likely that all are.
(See `JAVA-1252 <https://jira.mongodb.org/browse/JAVA-1252>`_.)
(See `JAVA-1252 <https://jira.mongodb.org/browse/JAVA-1252>`_,
`SPEC-1396 <https://jira.mongodb.org/browse/SPEC-1396>`_.)
Copy link
Member

Choose a reason for hiding this comment

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

SPEC tickets are not public, I don't think we can include this in our public documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@p-mongo p-mongo requested a review from mbroadst November 4, 2019 20:23
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.

5 participants