feat(proxy): waiting room requests #903
Conversation
There was a problem hiding this comment.
Dope! Had a first pass.
It seems there is still some gap when it comes to transition a request during clone attemtps, where the order would be: Found -> Cloning -> CloningFailed/TimedOut -> Cloning or Found -> Cloning -> Cloned. We want to make explicit when a request is being cloned to avoid scheduling it for anohter clone. Furthermore we should get our timeouts looked at, as we have the timeouts for a single operation and the terminal state of the request alltogether having timed out.
The dependency direction seems skewed with the waiting room depending on its super while being under the request module hierarchy. Maybe both request and waiting_room can be next to each other.
In its current form are we supporting repo requests for any repo not only projects? Or did I miss-understand the current imimplementation?
|
Would love to review this once ready 💯 |
xla
changed the title
A request is modeled as the data it needs to fulfill a request and the metadata that it encounters along its journey. We use a PhantomData marker to ensure that transitions between our states are valid. The waiting room is then modeled as a collection of these requests keyed by their RadUrn. The error handling is done within the waiting room since it needs to hide away what kind of request we had. There's a single happy path test to get us going.
Moving the waiting room into its own module to separate the boundary that it's a separate entity to request. This makes it easier to define the Error types locally and replace the () errors with these domain specific errors.
We add a Strategy enum so that the caller can pick which kind of strategy they want when selecting the next Request in a Created state.
We introduce the intermediate states, Found, Cloning, and Cloned at the type level so that we have more fine grained transitions. This means we can't have just PhantomData as the state. This introduces those changes and transitions while fixing the end-to-end test. However, there is still work to be done on the business logic that happens at each state and when we need to transition to a failure state such as canceled or timed out.
FintanH
force-pushed
the
fintan/tracking-notifications
branch
from
cca8c44
to
e0aebfd
Compare
We make more transitions explicit between request states and allow to go back to previous states. We also keep track of how a clone went for a given PeerId. Left some inline comments as reminder for what's next.
We capture shared functionality: Having peers and be able to cancel a request, and we organise them in traits. This allows us to give instances of these traits to the states that they are relevant for. This then means we can write generic transitions for Request. Since these traits are very particular they are sealed by this module.
We have some cases where it's easy to define the functions over a list of states. We also clean up some of the functionality and code.
The code is easier to browse if we split the state and the existential code into sub-modules.
Using deriving for Serialize and Deserialize for all the types so that we can ultimately serialize and deserialize a WaitingRoom.
Since I clearly made a mistake that was caught in code review, I won't let it happen again. It shall be caught at compile time! usize no more, only Queries and Clones!
While it is weird, the caller of the waiting room/request API has to hold themselves accountable in this case. If they say a peer failed then we mark it as failed, whether it existed in the map or not.
Introducing a method that checks the elapsed time between a request and provided timestamp.
We introduce a filter function that bases the filtering criteria off of the RequestKind and the delta of elapsed time based on a provided timestamp. This deprecates the next and ready functions.
There was a problem hiding this comment.
Excellent work, @FintanH 👏
left some comments but wouldn't mind if you skip them.
| pub const fn urn(&self) -> &RadUrn { | ||
| &self.urn | ||
| } |
There was a problem hiding this comment.
TIL on pub const
Out of curiosity, why having this getter instead of making urn public?
There was a problem hiding this comment.
I think it'd be disastrous if a user could change the RadUrn of the request in the WaitingRoom. #eNcApSuLaTiOn
There was a problem hiding this comment.
would that require it to be mut?
There was a problem hiding this comment.
Ya, it would I believe.
| /// Since a `Request` is parameterised over its state, it makes it difficult to talk about a | ||
| /// `Request` in general without the compiler complaining at us. For example, we cannot have | ||
| /// something like `vec![created, requested, cloning, timedout]` since they all have distinct types | ||
| /// where they differ in states. | ||
| /// | ||
| /// To allow us to do this we unify all the states into `SomeRequest` where each state is a variant | ||
| /// in the enumeration. | ||
| /// | ||
| /// When we pattern match we get back the request parameterised over the specific state and can | ||
| /// work in a type safe manner with this request. |
| } | ||
| } | ||
|
|
||
| impl<T, L: Into<SomeRequest<T>>, R: Into<SomeRequest<T>>> From<Either<L, R>> for SomeRequest<T> { |
There was a problem hiding this comment.
Would you like some explanation? :)
proxy/coco/src/request/states.rs
Outdated
| /// An enumeration of the different states a `Request` can be in. This is useful if we want to | ||
| /// convey the state information without any of the other state data. | ||
| #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] | ||
| pub enum RequestKind { |
There was a problem hiding this comment.
| pub enum RequestKind { | |
| pub enum RequestState { |
would be truer to its meaning and purpose.
There was a problem hiding this comment.
Ya, I'm in agreement with this 👍
| impl AddAssign<usize> for Queries { | ||
| fn add_assign(&mut self, other: usize) { | ||
| *self = Self(self.0 + other) | ||
| } | ||
| } |
FintanH
dismissed stale reviews from rudolfs, NunoAlexandre, and xla
via
474211d
Co-authored-by: Nuno Alexandre <hi@nunoalexandre.com>
…nto fintan/tracking-notifications
🚧 Under Construction 🚧
PeerIdReponextunder different strategiesRequestedstates, i.e.Found,Cloning, andClonedinto a type parameter as well ala this patternPart of ##803