[SERVER-50002] ReplicaSetAwareService test should just use dummy services Created: 29/Jul/20 Updated: 29/Oct/23 Resolved: 12/Aug/20 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Sharding |
| Affects Version/s: | None |
| Fix Version/s: | 4.7.0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Pierlauro Sciarelli | Assignee: | Kevin Pulo |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | PM-1645-Milestone-3 | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||
| Operating System: | ALL | ||||||||||||||||
| Sprint: | Sharding 2020-08-24 | ||||||||||||||||
| Participants: | |||||||||||||||||
| Linked BF Score: | 0 | ||||||||||||||||
| Description |
|
The ReplicaSetAwareService test is mistakenly ending up calling the VectorClockMongoD onStepUpComplete. So far the test has always worked because such method call always resulted in a NOP, but that will not be the case anymore with the changes that are going to be introduced in |
| Comments |
| Comment by Githook User [ 12/Aug/20 ] | ||||||||||||
|
Author: {'name': 'Kevin Pulo', 'email': 'kevin.pulo@mongodb.com', 'username': 'devkev'}Message: | ||||||||||||
| Comment by Githook User [ 12/Aug/20 ] | ||||||||||||
|
Author: {'name': 'Kevin Pulo', 'email': 'kevin.pulo@mongodb.com', 'username': 'devkev'}Message: | ||||||||||||
| Comment by Kaloian Manassiev [ 31/Jul/20 ] | ||||||||||||
|
kevin.pulo, passing this to you, since Pierlauro will be on vacation in the next couple of weeks. | ||||||||||||
| Comment by Andrew Morrow (Inactive) [ 31/Jul/20 ] | ||||||||||||
|
kevin.pulo - It is fine to add a new CppUnitTest with fewer dependencies if you must. Collapsing the unit test libraries was a desperation move in the first place, when we were still on the very slow VS2017 linker and when we didn't have --link-model=dynamic in the required waterfall and patch builders. Yes, adding the new binary will cost us something, but most likely not enough to adversely affect developer experience in a noticeable way. | ||||||||||||
| Comment by Kevin Pulo [ 31/Jul/20 ] | ||||||||||||
|
Argh, it's via db_repl_coordinator_test -> repl_coordinator_test_fixture -> db/service_context_d_test_fixture -> db/service_context_d -> db/s/sharding_runtime_d -> db/vector_clock_mongod. And a similar path exists for the other existing CppUnitTests under db/repl, topology_version_observer_test and db_repl_cloners_test. Prior to
So it looks like we need a "plain" unittest binary, that doesn't inherit any of the *_d libs — particularly not db/s/sharding_runtime_d, db/service_context_d, or db/service_context_d_test_fixture (though db/service_context and db/service_context_test_fixture will be needed). Since currently there doesn't seem to be any such CppUnitTest, we'll probably have to create it (client_out_of_line_executor_test appears to be precedent, though no clues explaining why it needs to be on its own). But since we try to minimise the number of separate binaries, we should check with the SDP Build team first. acm, thoughts? The only other possibility I can think of would be to remove the automatic registration of ReplicaSetAwareServices, and instead require manual registration during mongod/mongos/test fixture startup. The problem here is that since ServiceContext decorations are MONGO_INITIALIZER-based, this still might not allow different tests to have different ReplicaSetAwareServices... | ||||||||||||
| Comment by Pierlauro Sciarelli [ 30/Jul/20 ] | ||||||||||||
|
Unfortunately, even moving it under db_repl_coordinator_test doesn't work since vector_clock_mongod is somehow indirectly linked also there. EVG patch showing that the test still fails. | ||||||||||||
| Comment by Kevin Pulo [ 30/Jul/20 ] | ||||||||||||
|
So sharding_runtime_d is the only lib that vector_clock_mongod is linked to. In db/repl/, that lib (sharding_runtime_d) is only linked by rs_rollback, rollback_impl, oplog_application, and serveronly_repl. Of these 4, the db_repl_test unittest binary links directly to rs_rollback and rollback_impl, and to oplog_application via oplog_applier_impl_test_fixture. So this is how vector_clock_mongod is getting into db_repl_test, and it seems like we won't be able to break that link. The only places in db/repl/ that link replica_set_aware_service are repl_coordinator_impl, primary_only_service, and db_repl_test. primary_only_service_test.cpp is also in db_repl_test, so we can't move replica_set_aware_service_test.cpp to be alongside it. However, since the main consumer of replica_set_aware_service is repl_coordinator_impl, perhaps we can move replica_set_aware_service_test.cpp (and the corresponding replica_set_aware_service LIBDEP) out of db_repl_test, and instead into db_repl_coordinator_test? That unittest seems to have a deliberately limited set of libs, so should allow replica_set_aware_service_test.cpp to run without vector_clock_mongod. | ||||||||||||
| Comment by Kaloian Manassiev [ 30/Jul/20 ] | ||||||||||||
|
Is this a problem, because the VectorClock happens to be linked in ReplicaSetAwareService test binary. Most likely, it needs to be pulled into its own binary unfortunately. CC kevin.pulo. |