Refactor/303 round return storage actions

[kellpossible]

Perform refactor #303

Dependent on #361 (please do not merge before that)

[ibaryshnikov]

@kellpossible would you mind communicating the change you are making and what problem does it solve?

So far I can see that instead of calling methods of storage, function returns a vec of actions. It means that if the function fails half way, nothing gets written to the storage and it's easier to rollback.
From the other side, the vector of actions is processed sequentially, and if one action fails, what happens to the next one? Would it be easier to debug such an error when it happens directly in a function, for example try_lock, or in a separate function perform_actions?

[kellpossible]

would you mind communicating the change you are making and what problem does it solve?

@ibaryshnikov valid concerns. A few reasons:

1. Continuation of the changes made in Round restart #288 in partial response to Proposal: Refactor phase1-coordinator's coordinator.rs, coordinator_state.rs  #267, specifically moving side effects to a single location, and reducing coupling.
2. Easier to test, especially the functions which have removed Disk entirely, no need to mock in order to avoid file system interaction. Consider that it will now be trivial to unit test Round::remove_contributor_unsafe.

So far I can see that instead of calling methods of storage, function returns a vec of actions. It means that if the function fails half way, nothing gets written to the storage and it's easier to rollback.

That's an interesting benefit I hadn't considered.

From the other side, the vector of actions is processed sequentially, and if one action fails, what happens to the next one?

I guess that would depend on the code which takes the actions and performs the processing, they can decide whether failure of one action should prevent the execution of subsequent actions. It would be context specific.

Would it be easier to debug such an error when it happens directly in a function, for example try_lock, or in a separate function perform_actions?

At the moment they both have a logical root in some method of Coordinator, I suppose we could append extra information/context to the actions to help with debugging if needed. Perhaps some ergonomics of debugging are lost here, but it might also be gained in other ways, such as being easily able to inspect all the important mutations to the file system that are occurring as a result of invoking a Round method without relying on logging in every Disk method.

[kellpossible]

If we can write code that behaves more predictably, and explicitly (less hidden side effects), then hopefully we can reduce the amount of debugging required in the first place.

[kellpossible]

For some more context about these changes, I'm currently working on #365 which has some failing unit tests. As part of diagnosing/fixing this I would like to write some more unit tests for Round, many methods do not have good coverage.

[ibaryshnikov]

I'm quite skeptical about this change, because it doesn't address the cause of the problem, which is the lack of transactions when working with file system.

[kellpossible]

@ibaryshnikov the cause of which problem specifically? This pull request not claiming to help address anything more than the issues that have been referenced. Are you're worried that the changes pose an unacceptable risk to the system that is in production?

[ibaryshnikov]

the cause of which problem specifically?

the incorrect state of the transcript

Are you're worried that the changes pose an unacceptable risk to the system that is in production?

I don't think it does any harm, but it doesn't reduce the number of fs calls either

[kellpossible]

@ibaryshnikov I'm guessing we probably had the similar discussion on slack, but for the sake of others reviewing/reading, I'll reply here too. This is a quality of life refactor, only claiming to help address this issues referenced in the description and my comment above. It doesn't attempt to modify the observable functionality of the software or try to fix any bugs. Why is it important that this particular pull request addresses the incorrect state of the transcript and reduce the number of fs calls? Could not those issues be addressed in a separate pull request?

[kellpossible]

From the conversation in the meeting today, I should mention that this PR is not intended to introduce a universal way to interact with the storage layer, but more is designed to introduce a separation of concerns between managing the state of the Round, and interacting with the file system, and to make the Round code more explicit, more reliable and easier to test.

@ibaryshnikov you mentioned the use of Result types, and I didn't quite understand how it would solve the same issues that this PR aims to address, sorry we didn't give you time to elaborate on this. Would you be able to explain more about your thoughts? It's interesting that you mention the Result type, it's another design pattern which encourages returning explicit values (instead of hidden exception throw/raise side-effect behaviour) that many people who came from other languages were unfamiliar with, but which has quite a few benefits. I see this PR partly as an extension of the same idea.

This chapter https://livebook.manning.com/book/functional-programming-in-c-sharp-second-edition/chapter-3/v-5/122 (read quickly the free preview has a time limit eek) has a great overview of the problem that this PR is attempting to address. Rust avoids some of the pitfalls of impurity by requiring us to be explicate about the mutability of input arguments, however there still remains many problems with allowing &mut Storage to be passed into many modules/functions for which interaction with the file storage layer is a secondary concern.

I think the pattern used here is also a bit similar to the command design pattern. If you ignore the concerns about subclasses (we are using Rust after all!), conceptually replace the "User Interface" code with "Round" and replace "Business Logic" with "File System Operations" the analogy partly fits. What is different from their example is that the code to execute the command currently lives in the Disk module, rather than with the StorageAction implementation. It also demonstrates how using this design pattern neatly allows for roll-back/roll-forward if we want it (but that would require more extensive use of the pattern and there are other ways it could be achieved).

[kellpossible]

This PR now needs to be rebased