refactor(storage-manager): search replication log only once #1505
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `aligner.rs` file was only growing in size, rendering navigation
complicated. This commit splits it in the following manner:
- `aligner_query.rs` inherits the `AlignmentQuery` and the logic that
generates a reply to it.
- `aligner_reply.rs` inherits the `AlignmentReply` and the logic to
process it.
- `core.rs` inherits the `spawn_query_replica_aligner` that spawns a
task to query the "Aligner" of a Replica.
* plugins/zenoh-plugin-storage-manager/src/replication/aligner.rs:
deleted and split the content of the file.
* plugins/zenoh-plugin-storage-manager/src/replication/core.rs:
- Declared the new modules `aligner_query` and `aligner_reply`.
- Updated the imports as the method `spawn_query_replica_aligner`
was moved there.
* plugins/zenoh-plugin-storage-manager/src/replication/core/aligner_query.rs:
new file that holds the logic to generate a reply to an AlignmentQuery.
* plugins/zenoh-plugin-storage-manager/src/replication/core/aligner_reply.rs:
new file that holds the logic to process an AlignmentReply.
* plugins/zenoh-plugin-storage-manager/src/replication/mod.rs: removed
the, no longer used, module `aligner`.
Signed-off-by: Julien Loudet <[email protected]>
This commit adds the method `remove_older` to the `LogLatest`
structure. The motivation for this addition is to later optimise the
alignment by only searching through the Replication Log once when
replacing / adding Events.
To make the rest of the code more coherent, the method
`if_newer_remove_older` were renamed to `remove_older` for the
`Interval` and `SubInterval` structures.
* plugins/zenoh-plugin-storage-manager/src/replication/classification.rs:
- Renamed `if_newer_remove_older` to `remove_older` for the Interval
structure.
- Renamed `if_newer_remove_older` to `remove_older` for the
SubInterval structure.
- Added a line in the documentation of both `remove_older` methods to
indicate that their respective Fingerprint would be updated.
* plugins/zenoh-plugin-storage-manager/src/replication/log.rs:
- Updated the method `insert_event` to leverage the newly created
`remove_older` method.
- Added the `remove_older` method that removes from the Replication
Log an older Event on the same key expression.
* plugins/zenoh-plugin-storage-manager/src/replication/tests/classification.test.rs:
updated the calls to `remove_older` due to the renaming.
Signed-off-by: Julien Loudet <[email protected]>
This renaming makes the content of this variant clear: only the metadata is processed. * plugins/zenoh-plugin-storage-manager/src/replication/core/aligner_query.rs: renamed `AlignmentReply::Events` to `AlignmentReply::EventsMetadata`. * plugins/zenoh-plugin-storage-manager/src/replication/core/aligner_reply.rs: renamed `AlignmentReply::Events` to `AlignmentReply::EventsMetadata`, including in logging. Signed-off-by: Julien Loudet <[email protected]>
This commit moves, for clarity purposes, the logic to process an `EventMetadata` after receiving an `AlignmentReply::EventsMetadata` in a separate method: `process_event_metadata`. This notably removes indentation levels and makes the code more readable. * plugins/zenoh-plugin-storage-manager/src/replication/core/aligner_reply.rs: move the logic to process an `EventMetadat` in a separate method `process_event_metadata`. Signed-off-by: Julien Loudet <[email protected]>
This commit moves, for clarity purposes, the logic to process an `Retrieval` in a separate method: `process_event_retrieval`. This notably removes indentation levels and makes the code more readable. * plugins/zenoh-plugin-storage-manager/src/replication/core/aligner_reply.rs: move the logic to process a `Retrieval` in a separate method `process_event_retrieval`. Signed-off-by: Julien Loudet <[email protected]>
Before this commit, the Replication Log was searched twice when aligning
an Event: once while calling `lookup` and a second time when calling
`insert_event`.
To reduce the number of searches to a single one, the method
`insert_event_unchecked` was introduced. This function assumes that the
Replication Log contains no Event with the same key expression as the
Event it will insert.
Leveraging this, the calls to `LogLatest::lookup` followed by
`LogLatest::insert_event` were replaced with calls to
`LogLatest::remove_older` followed by
`LogLatest::insert_event_unchecked`.
Indeed, if the call to `LogLatest::remove_older` yields either
`NotFound` or `RemovedOlder` then the Replication Log does not contain
any event with the key expression, and
`LogLatest::insert_event_unchecked` can be confidently called.
Another consequence of the introduction of the new method
`insert_event_unchecked` is that the variant
`EventInsertion::NotInsertedAsOutOfBound` was no longer used and was
thus removed.
* plugins/zenoh-plugin-storage-manager/src/replication/core/aligner_reply.rs:
Replaced the calls to `lookup` followed by `insert_event` to calls
to `remove_older` followed by `insert_event_unchecked` in the
methods `process_event_metadata` and `process_event_retrieval`.
* plugins/zenoh-plugin-storage-manager/src/replication/log.rs:
- Removed the no longer used `EventInsertion::NotInsertedAsOutOfBound`
variant.
- Added a comment to clearly indicate that the method `insert_event`
will remove older Event.
- Introduced the new method `insert_event_unchecked` that inserts an
Event in the Replication Log without checking if it is the only
Event (i.e. it assumes it is the case).
Signed-off-by: Julien Loudet <[email protected]>
This commit introduces an assertion test every time an Event in inserted in the Replication Log ONLY IN DEBUG. The purpose is to make sure that the invariant of the Replication Log is upheld: it should only contain one Event per key expression. * plugins/zenoh-plugin-storage-manager/src/replication/classification.rs: added the method `assert_only_one_event_per_key_expr` to the `Interval` and `SubInterval` structures. * plugins/zenoh-plugin-storage-manager/src/replication/log.rs: added the method `assert_only_one_event_per_key_expr` to the `LogLatest` structure. Signed-off-by: Julien Loudet <[email protected]>
J-Loudet
force-pushed
the
refactor/storage-manager/search-replication-log-only-once
branch
from
dbb6aec to
f1a1dac
Compare
Charles-Schleich
approved these changes
Oct 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors the Aligner part of the Replication to (i) make the code easier to read and (ii) slightly optimize the alignment process by only searching the Replication Log once.
As a bonus, an
assert!was added when the storage manager is compiled in Debug to help us guarantee that the Replication Log only contains a single Event per key expression.