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

refactor!(storage-manager): remove replication to start-over #1406

Merged
merged 8 commits into from
Sep 19, 2024

Conversation

J-Loudet
Copy link
Contributor

The purpose of this pull request is to remove the previous implementation of the Replication feature in order to facilitate its rewrite.

See individual commits for details.

@J-Loudet J-Loudet added breaking-change Indicates that the issue implies a breaking change (be it at compile time or at runtime) internal Changes not included in the changelog labels Sep 12, 2024
@J-Loudet J-Loudet force-pushed the refactor/storage-manager/remove-replication branch 4 times, most recently from 8222b03 to 50bd9e0 Compare September 18, 2024 13:08
@J-Loudet J-Loudet marked this pull request as ready for review September 18, 2024 13:09
@J-Loudet
Copy link
Contributor Author

@Charles-Schleich
Copy link
Member

Charles-Schleich commented Sep 18, 2024

This PR looks good.
@J-Loudet shall we not also remove replica_config from DEFAULT_CONFIG.json5 ?

@J-Loudet J-Loudet force-pushed the refactor/storage-manager/remove-replication branch from 50bd9e0 to e48c4bd Compare September 18, 2024 15:28
The methods `create_and_start_storage` and `start_storage` were in two
separate files -- the former calling the latter.

This commit regroups them in the same file.

* plugins/zenoh-plugin-storage-manager/src/backends_mgt.rs: deleted the
  file, exporting its content to `storages_mgt.rs`.
* plugins/zenoh-plugin-storage-manager/src/lib.rs: fixed the imports.
* plugins/zenoh-plugin-storage-manager/src/replica/mod.rs: fixed the
  imports.
* plugins/zenoh-plugin-storage-manager/src/replica/storage.rs: fixed the
  imports.
* plugins/zenoh-plugin-storage-manager/src/storages_mgt.rs: imported the
  code contained in `backends_mgt.rs`.

Signed-off-by: Julien Loudet <[email protected]>
…_storage`

* plugins/zenoh-plugin-storage-manager/src/storages_mgt.rs: merged the two
  functions together.

Signed-off-by: Julien Loudet <[email protected]>
This commit is a first step towards a clearer separation between the
management of a storage and the replication feature.

No internal logic or code was modified. This commit only offers a first,
incomplete, level of separation between the two functionalities.

* plugins/zenoh-plugin-storage-manager/src/replica/mod.rs:
  - fixed the import statements,
  - created a `service` module.
* plugins/zenoh-plugin-storage-manager/src/replica/service.rs:
  - moved the `ReplicationService` structure there.
* plugins/zenoh-plugin-storage-manager/src/storages_mgt/mod.rs:
  - fixed the import statements after the move,
* plugins/zenoh-plugin-storage-manager/src/storages_mgt/service.rs:
  - moved the `StorageService` structure and implementation there,
  - fixed the import statements,
  - removed the `ReplicationService` structure.

Signed-off-by: Julien Loudet <[email protected]>
BREAKING CHANGE: the replication feature no longer exists.

Sometimes, starting from scratch is easier…

The code of the storage functionality and of the replication were
completely entangled.

The main purpose of this commit is to clearly separate both. A second
objective is to bootstrap the rewrite of the replication.

Note that the code of the replication was not deleted from the
repository as it will be used as a basis for the rewrite.

* Cargo.lock: (see Cargo.toml).

* DEFAULT_CONFIG.json5: removed the `replica_config` section.

* plugins/zenoh-backend-traits/src/config.rs: removed the
  `ReplicaConfig` structure.

* plugins/zenoh-plugin-storage-manager/Cargo.toml:
  - removed the unused dependencies:
    - crc
    - derive-new
    - serde
  - sorted alphabetically `tracing`.
* plugins/zenoh-plugin-storage-manager/src/lib.rs: removed the `replica`
  module.
* plugins/zenoh-plugin-storage-manager/src/storages_mgt/mod.rs: removed
  the call to the Replica service.
* plugins/zenoh-plugin-storage-manager/src/storages_mgt/service.rs:
  removed the replica related code
  - removed the `replication` field of the `StorageService` structure,
  - removed the `initialize_if_empty` method that was only used when a
    storage was configured to replicate its data,
  - removed the duplicated code in the main loop of the
    `start_storage_queryable_subscriber`.
* plugins/zenoh-plugin-storage-manager/src/replica/align_queryable.rs:
  deleted.
* plugins/zenoh-plugin-storage-manager/src/replica/aligner.rs: deleted.
* plugins/zenoh-plugin-storage-manager/src/replica/digest.rs: deleted.
* plugins/zenoh-plugin-storage-manager/src/replica/mod.rs: deleted.
* plugins/zenoh-plugin-storage-manager/src/replica/service.rs: deleted.
* plugins/zenoh-plugin-storage-manager/src/replica/snapshotter.rs:
  deleted.

Signed-off-by: Julien Loudet <[email protected]>
Fixing the typos led to format changes.

* plugins/zenoh-plugin-storage-manager/src/lib.rs:
  - voulme -> volume
  - admoin -> admin
* plugins/zenoh-plugin-storage-manager/src/storages_mgt/service.rs:
  - ovderriding -> overriding
  - makeit -> make it

Signed-off-by: Julien Loudet <[email protected]>
This structure was only created once to instantly be "split".

* plugins/zenoh-plugin-storage-manager/src/storages_mgt/mod.rs:
  - removed the unused `Capability` import,
  - removed the structure `StoreIntercept`,
* plugins/zenoh-plugin-storage-manager/src/storages_mgt/service.rs:
  - removed the `StoreIntercept` import,
  - changed the signature of the `start` method to take the `storage`
    and the `capability` as argument instead of the `StoreIntercept`.

Signed-off-by: Julien Loudet <[email protected]>
The replication service needs to access the underlying `dyn Storage` in
a separate task and mutate its internal state.

This commit simply changes the location where the storage is wrapped
inside an `Arc<Mutex<_>>`.

* plugins/zenoh-plugin-storage-manager/src/storages_mgt/mod.rs: wrap
  the `dyn Storage` inside an `Arc<Mutex<_>>` before spawning the
  `StorageService`.
* plugins/zenoh-plugin-storage-manager/src/storages_mgt/service.rs:
  update the signature of the method `start` to accept an
  `Arc<Mutex<Box dyn Storage>>` instead of a `Box<dyn Storage>`.

Signed-off-by: Julien Loudet <[email protected]>
This is mainly to avoid confusion with the crate of the same name.

* plugins/zenoh-plugin-storage-manager/src/storages_mgt/mod.rs: renamed
  `zenoh` into `zenoh_session`.

Signed-off-by: Julien Loudet <[email protected]>
@J-Loudet J-Loudet force-pushed the refactor/storage-manager/remove-replication branch from e48c4bd to 8eddaaa Compare September 18, 2024 15:29
@J-Loudet
Copy link
Contributor Author

This PR looks good. @J-Loudet shall we not also remove replica_config from DEFAULT_CONFIG.json5 ?

Good catch!
I have updated the commit ee86277 and removed the ReplicaConfig structure as well as the in the DEFAULT_CONFIG.json5.

@J-Loudet J-Loudet merged commit db755e3 into main Sep 19, 2024
24 checks passed
@J-Loudet J-Loudet deleted the refactor/storage-manager/remove-replication branch September 24, 2024 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Indicates that the issue implies a breaking change (be it at compile time or at runtime) internal Changes not included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants