[SERVER-44510] Service layer / command changes to implement exhaust isMaster Created: 08/Nov/19 Updated: 08/Jan/24 Resolved: 16/Dec/19 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Replication |
| Affects Version/s: | None |
| Fix Version/s: | 4.3.3 |
| Type: | New Feature | Priority: | Major - P3 |
| Reporter: | A. Jesse Jiryu Davis | Assignee: | Tess Avitabile (Inactive) |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||||||||||||||||||
| Sprint: | Repl 2019-12-02, Repl 2019-12-16, Repl 2019-12-30 | ||||||||||||||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||||||||||||||
| Description |
|
See design for details. We do not need to ensure that if the server detects a client disconnect while waiting to send a reply, the server cleans up connection resources, since that will be done in Log at log level 3 whether exhaust is used. |
| Comments |
| Comment by Githook User [ 16/Dec/19 ] | |||
|
Author: {'name': 'Tess Avitabile', 'email': 'tess.avitabile@mongodb.com', 'username': 'tessavitabile'}Message: | |||
| Comment by Tess Avitabile (Inactive) [ 04/Dec/19 ] | |||
|
Thank you, all! I was unaware of the C++ integration tests, and I agree that this is the way to go. (Note that we do have unit test coverage of exhaust getMore in service_state_machine_test.cpp, but these tests mock out the ServiceEntryPoint, so they only test the piece of the implementation in makeExhaustMessage()). | |||
| Comment by Matthew Russotto [ 04/Dec/19 ] | |||
|
I think the best way to test this is a C++ integration test. Something like the one Lingzhi mentioned (from | |||
| Comment by Lingzhi Deng [ 03/Dec/19 ] | |||
|
And we also have https://github.com/mongodb/mongo/blob/3c77881955004780a8dd424434575f9dc0a5632d/src/mongo/rpc/op_msg_integration_test.cpp#L291 written by Will as part of | |||
| Comment by A. Jesse Jiryu Davis [ 03/Dec/19 ] | |||
|
However, the other possibility is to add a C++ integration test using DBClientConnection, here: I believe this test is executed with a running mongod (or replica set or sharded cluster) available. So maybe this is an even faster route to testing exhaust ismaster. mira.carey@mongodb.com do I understand this test correctly? | |||
| Comment by A. Jesse Jiryu Davis [ 03/Dec/19 ] | |||
|
I recall thinking it'd take perhaps a week to add shell support for exhaust as I sketched above, for me or someone like me with a small bit of experience with the shell's C++ layer. That'll give us the kind of JSTest support for exhaust commands that we're accustomed to for other commands, so it's not merely the fastest route to testing exhaust isMaster, it might also be the "best". | |||
| Comment by Tess Avitabile (Inactive) [ 03/Dec/19 ] | |||
|
jesse, you were correct above that we don't have a way to test exhaust isMaster currently. I had hoped we could write unit tests in service_state_machine_test.cpp, but this mocks out the ServiceEntryPoint and only tests the functionality in makeExhaustMessage(). Since the next invocation will be constructed in CmdIsMaster, we would not be able to test that it is formed correctly in service_state_machine_test.cpp. I have two ideas for how we can test exhaust isMaster. The first is your suggestion above to add minimal exhaust command support in the shell. The second is to try to test exhaust isMaster in unit tests or dbtests. We could make DBDirectClient support exhaust, but that sounds hard, since DBDirectClient bypasses the ServiceStateMachine. DBClientConnection supports exhaust, but it requires us to have a running server, which we don't in the context of a unit test or dbtest. I feel a little over my head on either of these approaches. jesse, mira.carey@mongodb.com, I would appreciate your guidance. | |||
| Comment by Mira Carey [ 03/Dec/19 ] | |||
|
From a quick scan, it's not clear to me how much extra parsing/scanning of bson we'd have to do as part of the tess' proposal. If a small amount, my gut would be to deserialize/serialize, because I wouldn't expect the overhead to be large enough to produce a regression. If the serialize/deserialize proposal has another full validate in it... I might still start off with that one. Make the change, verify that we actually have the regression, then try and fix it (rather than optimizing before we know the size of the problem). But in that case I'd expect to have to look into this again a little later down the road (given that | |||
| Comment by Tess Avitabile (Inactive) [ 03/Dec/19 ] | |||
|
I agree that serializing the next response will make exhaust cursors on the oplog slower due to This deserialization that jesse pointed out would go away in Ideally, we would pass a parsed request to Command::run(), so that we can just modify the current parsed request and not need to do any serialization/deserialization, but that would be a large change. | |||
| Comment by A. Jesse Jiryu Davis [ 03/Dec/19 ] | |||
|
Interesting. Let's consider deserializing first. I think we re-parse the getMore request twice per exhaust cursor reply, once in GetMoreCmd and once here: https://github.com/mongodb/mongo/blob/r4.3.2/src/mongo/transport/service_state_machine.cpp#L107 That might not be optimal, but I believe we can avoid making it any worse if we wrap the request's BSON body in an InvocationArgs struct. Today, I think that we don't have to serialize the getMore request at all, we keep reusing the first request. I hope we can preserve that behavior even when the request is wrapped in an InvocationArgs. | |||
| Comment by Lingzhi Deng [ 03/Dec/19 ] | |||
|
And I think in general (probably not for this ticket?), it might be a good idea to minimize the appearance of BSONObj in the codebase when it isn't referring to an actual document in the database. And I think this could give us more flexibility in the long run. | |||
| Comment by A. Jesse Jiryu Davis [ 03/Dec/19 ] | |||
|
I don't think we should worry much about performance. I agree with Tess that we don't expect a regression in exhaust cursor performance, and we won't see it in regular ismaster either if we ensure that ismaster only does the extra BSON serialization work if exhaust is set. Exhaust ismaster will only generate InvocationArgs occasionally so it need not be very high-performance. It sounds to me like the performance gain of Lingzhi's proposal is not worth the difficulty of implementing it. Furthermore, it might be harder to understand and maintain than the alternative, which is to serialize the next invocation as BSON. I think the alternative approach is direct and easy to understand and maintain and will perform well enough. I don't see great value in choosing a more complex solution than this. | |||
| Comment by Lingzhi Deng [ 02/Dec/19 ] | |||
|
To reduce redundant information in my proposal, maybe we can form a “ghost” synthetic request body once (something like {isExhaust: true}) for exhaust requests and make changes to Command::parse to “parse” from the invocation args instead of from the request body BSON. But this will require more work. Just something to consider. | |||
| Comment by Tess Avitabile (Inactive) [ 02/Dec/19 ] | |||
|
We have two proposals for how a command can pass arguments to its next invocation when exhaust is enabled. I proposed that the current invocation create the BSON for the next invocation and set this in the ReplyBuilderInterface. The next invocation will receive no additional arguments. lingzhi.deng proposed that the current invocation set command-specific arguments as an InvocationArgs in the ReplyBuilderInterface. The next invocation will use the original BSON request for its next invocation. A downside to my proposal is the cost of serializing and deserializing BSON. I don't think it would cause a performance regression in exhaust cursors, since we could use the BSON from the current invocation for the next invocation. However, for exhaust isMaster, we would need to reserialize with the latest topologyVersion. To avoid a regression in isMaster without exhaust, we would want to ensure we only do this when exhaust is set. A downside to lingzhi.deng's proposal is that there will be redundant information in the next invocation's BSON request and InvocationArgs. The next invocation will have to ignore the topologyVersion in the BSON request and know to use the topologyVersion in the InvocationArgs. Additionally, we will have to decide how to pass the InvocationArgs to the next invocation. We could put this in ReplyBuilderInterface, but then ReplyBuilderInterface feels like an in/out parameter. I think we should go with lingzhi.deng's approach for performance, but I'm open to other points of view. I will also ask someone from Service Architecture to weigh in and participate when I put up a code review. | |||
| Comment by Haley Connelly [ 25/Nov/19 ] | |||
|
After speaking with jesse, we've decided that this ticket will be split up into two parts. (1) Introduce InvocationArgs and getNextInvocation() and setNextInvocation() methods to the ReplyBuilder interface. Additionally, overload BasicCommand::run to take a ReplyBuilderInterface. Part 1 of the code should not include tests, since we must wait for TopologyCoordinator::awaitTopologyChange to be implemented and use the future's topologyVersion for setNextInvocation(). (2) Implement logic for setNextInvocation() to be called isMaster exhaust is flagged, as well as store the nextInvocation inside the DbResponse. Write tests and ensure the InvocationArgs meet the exhaust isMaster design details, using the intended topologyVersion. Since my rotation is ending soon, I will work on part 1 before this ticket is handed to someone else. | |||
| Comment by Tess Avitabile (Inactive) [ 22/Nov/19 ] | |||
|
Another option is to test this in unit tests, as in | |||
| Comment by A. Jesse Jiryu Davis [ 21/Nov/19 ] | |||
|
To test this we need some client that sets exhaustAllowed in its isMaster request. Eventually drivers and the ReplicaSetMonitor will do this as part of their SDAM implementation, but not soon. Perhaps we could add minimal exhaust command support in the shell, to allow a test like this?:
The work would be mainly in MongoBase::Functions::runCommandWithMetadata. The MongoBase object would need a new method "receive", and it'd need to track whether it is currently streaming exhaust replies or not. |