Skip to content
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

CXX-336 remove connection pool and associated cruft #167

Merged
merged 1 commit into from Oct 3, 2014

Conversation

amidvidy
Copy link
Contributor

includes a small number of trailing whitespace removal noise, but given the pretty large changes to the files involved, I think its acceptable


// Assert here instead of returning NULL since the contract of this method is such
// that returning NULL means none of the nodes were good, which is not the case 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.

this comment should probably stay.

@acmorrow
Copy link
Contributor

Adam overall this looks good. I have one general comment and two high level concerns:

  • RE whitespace only changes, don't go back and revert those (unless very easy to do so, for all of them), but in the future resist the temptation, since it confuses things like git blame. If possible, do them in separate commits if you think they are warranted. In general, I don't think they are worth going back and fixing, though of course we should be ruthless about not allowing new ones to be introduced.
  • We are now requiring users of the library who want pooling to implement it themselves. This is fine and I think in general it is the right thing to do. However, I'd like to ensure that it is actually possible for them to do so. In particular: did the previous pool implementation utilize any non-public information about connections that would preclude a user of the library from re-creating the previous pool behavior? Can you provide an argument about whether or not this is possible post this change?
  • I'm somewhat concerned about the cost of creating new connections in the regime of expensive auth mechanisms like SCRAM. Can you make an argument about how a user of the library will be affected by expensive authorization mechanisms with the pool removed? In particular, I'm interested in the cost of connections internal to the library, since assuming that the resolution of the above point is satisfactory a client of the library could obviate the auth cost by building their own pool. However, there is nothing they can do about connections being constructed internally by the library.

@amidvidy
Copy link
Contributor Author

re: whitespace changes, I think I'm just going to bite the bullet and undo them

I don't think it should be hard to implement a pool. The DBClient supplies a public sStillConnected() method to check if a connection is still up. The rest of the work required is just constructing a threadsafe mapping of HostAndPort to List/Stack of DBClientConnection objects, which is still entirely doable.

Regarding cost of creating/destroying connections, this is definitely a concern. The ReplicaSetMonitor relies on the pool to cheaply contact monitored hosts in RSM::Refresher::_refreshUntilMatches. With SCRAM this could be overly expensive. I think a stopgap would be to implement a pared down pool, private to the ReplicaSetMonitor and make ScopedDbConnection completely internal to the library. I will try to rework this PR in that direction.

@amidvidy
Copy link
Contributor Author

So I added a connection caching mechanism to ReplicaSetMonitor. It's pretty simple and should amortize the cost of frequently executing isMaster on RS members while refreshing RS state. It also serves as a simple example of how an end-user of the driver could implement a pool since it uses no non-public methods of DBClientConnection.

// that returning NULL means none of the nodes were good, which is not the case here.
uassert(16532, str::stream() << "Failed to connect to " << _lastSlaveOkHost.toString(),
newConn != NULL);
uassert(17889, "Unable to construct DBClientConnection", _lastSlaveOkConn.get());
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did this uassert code come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, just going to use 0.

@acmorrow
Copy link
Contributor

acmorrow commented Oct 3, 2014

This is looking good. I want to chat about the conn cache a little bit and maybe do one more round: I think it may be able to be simplified somewhat

@amidvidy
Copy link
Contributor Author

amidvidy commented Oct 3, 2014

removed whitespace changes, cleaned up conn cache, and removed unnecessary null checks. @acmorrow let me know what you think.

const HostAndPort& _host;
};

// Caller must hold cache lock.
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this comment is true anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, fixed.

@acmorrow
Copy link
Contributor

acmorrow commented Oct 3, 2014

LGTM. Ship it!

@amidvidy amidvidy merged commit 6fb1d7f into mongodb:legacy Oct 3, 2014
@amidvidy
Copy link
Contributor Author

amidvidy commented Oct 3, 2014

sweet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants