Skip to content

JAVA-2057: Do not create pool when SUGGEST_UP topology event received#1149

Merged
tolbertam merged 3 commits into
4.xfrom
java2057
Dec 13, 2018
Merged

JAVA-2057: Do not create pool when SUGGEST_UP topology event received#1149
tolbertam merged 3 commits into
4.xfrom
java2057

Conversation

@tolbertam
Copy link
Copy Markdown
Contributor

@tolbertam tolbertam commented Dec 12, 2018

For JAVA-2057.

This should fix an issue I noticed on our CI servers, were occasionally after forcing a node down I observed that there were still some open connections. I have CI running repeatedly against #1147, which has this fix, so assuming that continues to run well we should consider merging this. Lets wait overnight to see how things go.

Old description (see 41dec29 for actual change)

Motivation:

There is a small window where a NEW_NODE event may be sent over the
control connection while a pool is initializing. This is more likely to
happen in testing, where a cluster is brought up and the driver connects
to it immediately.

In this case, the driver would erroneously create another pool, which
would create additional connections. The first pool created between
the currently initializing one and the new one being created would be
untracked with its connections remanining open even after the Session is
closed.

Modifications:

Akin to the logic in processDistanceEvent and processStateEvent, only
invoke createOrReconnectPool in onTopologyEvent when there is not an
initializing pool future present in the pending map.

Result:

createOrReconnectPool is no longer invoked in onTopologyEvent if there
is a pending initialization for that node.

@tolbertam tolbertam requested a review from olim7t December 12, 2018 21:49
@tolbertam tolbertam added this to the 4.0.0-beta3 milestone Dec 12, 2018
@tolbertam
Copy link
Copy Markdown
Contributor Author

Ack! This may not completely fix the issue..looks like the issue has recurred even with this fix, so I have more investigation to do.

@tolbertam tolbertam added the wip label Dec 12, 2018
@tolbertam
Copy link
Copy Markdown
Contributor Author

Ack! This may not completely fix the issue..looks like the issue has recurred even with this fix, so I have more investigation to do.

The problem was we never put the pool in pending in the typical initialization case. Doesn't that seem like a problem too? In the case of processDistanceEvent / processStateEvent being received during pool initialization, couldn't that be a problem too?

@tolbertam
Copy link
Copy Markdown
Contributor Author

I see, I don't think that will be a problem...we probably need a pool for those distance / state events to even happen? Although I do find it confusing that we have a pending map that doesn't consider pool initialization.

@tolbertam tolbertam changed the title JAVA-2057: Ignore SUGGEST_UP TopologyEvent if pool is pending JAVA-2057: Do not create pool when SUGGEST_UP topology event received Dec 13, 2018
@tolbertam
Copy link
Copy Markdown
Contributor Author

Made the recommended adjustment and rephrased the PR. If this is good I will also alter the CHANGELOG & commit log on merge

@olim7t
Copy link
Copy Markdown
Contributor

olim7t commented Dec 13, 2018

In the case of processDistanceEvent / processStateEvent being received during pool initialization, couldn't that be a problem too?

Those events are coalesced and recorded in pendingDistanceEvents/pendingStateEvents, and replayed in onPoolReady() once the pool is done initializing.

@olim7t
Copy link
Copy Markdown
Contributor

olim7t commented Dec 13, 2018

Yeah your change is what I had in mind. Does that resolve all issues in CI?

@tolbertam
Copy link
Copy Markdown
Contributor Author

Yeah your change is what I had in mind. Does that resolve all issues in CI?

It looks like it! Have had enough consecutive runs without the error reproducing to feel comfortable with this. I'm going to push a commit that will update change log and have the final full commit message to reflect what was done here. Cool if I merge afterwords and also merge in #1147 after?

@olim7t
Copy link
Copy Markdown
Contributor

olim7t commented Dec 13, 2018

Works for me 👍
I assume you plan to clean up the history in #1147, looks like you reverted a few things along the way.

@tolbertam
Copy link
Copy Markdown
Contributor Author

Definitely will clean that up, it's a bit of a mess at the moment 😄

Motivation:

There is a small window where a NEW_NODE event may be sent over the
control connection while a pool is initializing.  This is more likely to
happen in testing, where a cluster is brought up and the driver connects
to it immediately.

In this case, the driver would erroneously create another pool, which
would create additional connections.  The first pool created between
the currently initializing one and the new one being created would be
untracked with its connections remanining open even after the Session is
closed.

Modifications:

Akin to the logic in processDistanceEvent and processStateEvent, only
invoke createOrReconnectPool in onTopologyEvent when there is not an
initializing pool future present in the pending map.

Result:

createOrReconnectPool is no longer invoked in onTopologyEvent if there
is a pending initialization for that node.
Motivation:

There is a small window where a NEW_NODE event may be sent over the
control connection while a pool is initializing. This is more likely to
happen in testing, where a cluster is brought up and the driver connects
to it immediately.

In this case, the driver would erroneously create another pool, which
would create additional connections. The first pool created between
the currently initializing one and the new one being created would be
untracked with its connections remanining open even after the Session is
closed.

Modifications:

Change onTopologyEvent to no longer call createOrReconnectPool, instead
only call reconnectNow() if there is an already established pool.
initializing pool future present in the pending map.

Result:

SUGGEST_UP events now only trigger reconnect on existing pool, and will
no longer create a new pool.
@tolbertam tolbertam removed the wip label Dec 13, 2018
@tolbertam tolbertam merged commit 6794c38 into 4.x Dec 13, 2018
@tolbertam tolbertam deleted the java2057 branch December 13, 2018 19:47
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