Hide implementation details of our impl/mock subclasses behind static getters on the base class

XMLWordPrintableJSON

    • Type: Task
    • Resolution: Unresolved
    • Priority: Major - P3
    • None
    • Affects Version/s: None
    • Component/s: None
    • Replication
    • None
    • None
    • None
    • None
    • None
    • None
    • None

      The following classes have public/open modularity visibility, even though the actual implementation details of the class are not relevant:

       $ rg "MONGO_MOD_(PUB|OPEN) \w+(Impl|Mock)( |\$)" src/mongo/db/repl
src/mongo/db/repl/replication_coordinator_external_state_mock.h
67:class MONGO_MOD_PUB ReplicationCoordinatorExternalStateMock

src/mongo/db/repl/replication_coordinator_impl.h
185:class MONGO_MOD_PUB ReplicationCoordinatorImpl : public ReplicationCoordinator,

src/mongo/db/repl/replication_coordinator_external_state_impl.h
75:class MONGO_MOD_PUB ReplicationCoordinatorExternalStateImpl final

src/mongo/db/repl/replication_recovery.h
95:class MONGO_MOD_PUB ReplicationRecoveryImpl : public ReplicationRecovery {

src/mongo/db/repl/storage_interface_mock.h
100:class MONGO_MOD_PUB StorageInterfaceMock : public StorageInterface {

src/mongo/db/repl/replication_recovery_mock.h
39:class MONGO_MOD_PUB ReplicationRecoveryMock : public ReplicationRecovery {

src/mongo/db/repl/storage_interface_impl.h
62:class MONGO_MOD_OPEN StorageInterfaceImpl : public StorageInterface {

src/mongo/db/repl/data_replicator_external_state_impl.h
67:class MONGO_MOD_OPEN DataReplicatorExternalStateImpl : public DataReplicatorExternalState {

src/mongo/db/repl/replication_coordinator_mock.h
93:class MONGO_MOD_OPEN ReplicationCoordinatorMock : public ReplicationCoordinator {

      For example, StorageInterfaceImpl has public visibility, so that people can write code like this:

      #include "mongo/db/repl/storage_interface_impl.h"

    repl::StorageInterface::set(getServiceContext(),
                                std::make_unique<repl::StorageInterfaceImpl>());

      However, the only valid thing we expect people on other teams to do with a StorageInterfaceImpl or a StorageInterfaceMock is create it.
      They need not even know of the existence of these classes. There should just be two factory functions that both return a pointer to StorageInterface, and one function should be named "mock" and another function should be named "impl".

      By doing this we would:

      • have a better separation of our public and private API
      • make our code more readable
      • use decorators to create or retrieve dependencies. For example, if we would normally unconditionally pass in a std::unique_ptr<StorageInterface>, then we would instead make that a default argument, and then make the default value would be something like this (for a mock interface):
      std::unique_ptr<StorageInterface> defaultValue = [&]() {
    auto storageInterface = StorageInterface::get(svcCtx);
    if (storageInterface != nullptr) {
        return std::make_unique<StorageInterface>(storageInterface);
    }
    storageInterface = repl::mock::StorageInterface(svcCtx);
    StorageInterface::set(svcCtx);
    return storageInterface;
}();

      Below is an example of how this could simplify our code:

      std::unique_ptr<repl::ReplicationCoordinator>
AttachedServiceLifecycle::initializeReplicationCoordinator(ServiceContext* svcCtx) {
    auto storageInterface = repl::StorageInterface::get(svcCtx);
    auto replicationProcess = repl::ReplicationProcess::get(svcCtx);

    repl::TopologyCoordinator::Options topoCoordOptions;
    topoCoordOptions.maxSyncSourceLagSecs = Seconds(repl::maxSyncSourceLagSecs);
    topoCoordOptions.clusterRole = serverGlobalParams.clusterRole;

    return std::make_unique<repl::ReplicationCoordinatorImpl>(
        svcCtx,
        getGlobalReplSettings(),
        std::make_unique<repl::ReplicationCoordinatorExternalStateImpl>(
            svcCtx, storageInterface, replicationProcess),
        makeReplicationExecutor(svcCtx),
        std::make_unique<repl::TopologyCoordinator>(topoCoordOptions),
        replicationProcess,
        storageInterface,
        SecureRandom().nextInt64());
}

      std::unique_ptr<repl::ReplicationCoordinator>
AttachedServiceLifecycle::initializeReplicationCoordinator(ServiceContext* svcCtx) {

    repl::TopologyCoordinator::Options topoCoordOptions;
    topoCoordOptions.maxSyncSourceLagSecs = Seconds(repl::maxSyncSourceLagSecs);
    topoCoordOptions.clusterRole = serverGlobalParams.clusterRole;

    return repl::impl::ReplicationCoordinator(
        svcCtx,
        // note: replication coordinator external state deduced to be impl
        repl::impl::TopologyCoordinator(topoCoordOptions));
}

            Assignee:
            Unassigned
            Reporter:
            Joseph Obaraye
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated: