-
Notifications
You must be signed in to change notification settings - Fork 26
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
Reorganize daphne_worker #375
Conversation
a37342e
to
90a0b95
Compare
90a0b95
to
e0fa341
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, just a few things to clean up in my opinion. (All open for discussion, of course.)
daphne/src/roles/leader.rs
Outdated
resp_media_type: DapMediaType, | ||
resource: DapResource, | ||
req_data: Vec<u8>, | ||
is_put: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If set, this PUTs instead of POSTs. Since we're changing this code, we might as well rename it to be more accurate. How about "leader_send_http_request" and "LeaderHttpRequestOptions"?
daphne_worker/src/durable/mod.rs
Outdated
req: Request, | ||
state: &State, | ||
env: &Env, | ||
deployment: crate::config::DaphneWorkerDeployment, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Suggestion:
staet
,env
,deployment
, andtouched
are attributes common to all DOs that are "garbage collected". WDYTA about defining a trait for these and haverun_garbage_collection
be a derived method on this trait? I think this would make the code a lot cleaner. - This doesn't actually "run" garbage collection: It puts the DO instance on the garbage collection queue, which is dequeued before each e2e tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear: The second suggestion is to rename the method.
a723785
to
5fa4693
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :) Just a few nits.
daphne/src/roles/leader.rs
Outdated
path, | ||
req_media_type, | ||
resp_media_type, | ||
resource, | ||
req_data, | ||
is_put, | ||
method: is_put, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/is_put/method
.await? | ||
{ | ||
ControlFlow::Continue(req) => req, | ||
// this req was a GC request and as such we must return from this funcition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// this req was a GC request and as such we must return from this funcition. | |
// This req was a GC request and as such we must return from this function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Here and below)
&self.state | ||
} | ||
|
||
fn deployment(&self) -> crate::config::DaphneWorkerDeployment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It would be slightly more general to return a reference to self..config
, i.e., change the signature here to config(&self) -> &Confg
(I forget the name of the actual type at the meoment)
5bf7976
to
6fdeca3
Compare
Macros make the code harder to read as there are no types inside a macro body. They are also harder to debug and auto compleation doesn't work inside them.
6fdeca3
to
11e3e17
Compare
11e3e17
to
c0272cb
Compare
This PR cleans up the codebase a bit more by:
This PR is best viewed one commit at a time!