Drop Storage abstraction, revise fs cleanup logic for round reset
#361
Conversation
jules
requested review from
apruden2008,
emmorais,
howardwu,
ibaryshnikov,
kellpossible and
zosorock
|
Since it also touches a lot of files, I would prefer to not merge the storage refactoring before we merge #363, otherwise it may eliminate the work I've put into fixing tests. |
| self.clear_dir_files(round_dir.into(), false) | ||
| } | ||
|
|
||
| fn clear_dir_files(&mut self, path: PathBuf, delete_initial_contribution: bool) -> Result<()> { |
There was a problem hiding this comment.
I really like this approach. Resetting the round looks like a well defined problem after checking the folder structure.
| } | ||
|
|
||
| impl Storage for Disk { | ||
| impl Disk { |
There was a problem hiding this comment.
could this be in the same impl block as the one below?
|
|
||
| impl Storage for Disk { | ||
| impl Disk { | ||
| pub fn clear_round_files(&mut self, current_round_height: u64) -> Result<()> { |
There was a problem hiding this comment.
this doesn't necessarily have to be the current round? could perhaps just be round_height?
| /// the [crate::storage::Storage] to reflect the changes to the | ||
| /// round state. `remove_participants` is a list of participants | ||
| /// to remove from the round. | ||
| pub(crate) fn reset(&mut self, remove_participants: &[Participant]) -> Vec<StorageAction> { |
There was a problem hiding this comment.
nice to see lots of code getting deleted :) despite having worked so hard on this!
| self.storage.process(round.reset(&reset_action.remove_participants))?; | ||
|
|
||
| // Clear all files | ||
| self.storage.clear_round_files(current_round_height)?; |
There was a problem hiding this comment.
I think it would be good to represent clear_round_files as a storage action, returned by round.reset() instead of calling it directly here. I think it better fits with the existing pattern and improves testability.
| /// Returns the size of the object stored at the given locator. | ||
| fn size(&self, locator: &Locator) -> Result<u64, CoordinatorError>; | ||
|
|
||
| /// Process a [StorageAction] which mutates the storage. |
There was a problem hiding this comment.
I noticed that this description is missing from the process() implementation in Disk, if any other of these doc comments are missing, would be good to copy them across
jules
force-pushed
the
simplify_storage
branch
from
7b71127 to
33ca464
Compare
|
@julesdesmit I guess you're good to merge this one? |
Fixes #360