-
Notifications
You must be signed in to change notification settings - Fork 11
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
docs: add documentation for component server #222
docs: add documentation for component server #222
Conversation
d73680c
to
c5b88b9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## pr/Itay-Tsabary-Starkware/tsabary/refactor_mempool_infra/d3030c87 #222 +/- ##
==================================================================================================
Coverage 83.20% 83.20%
==================================================================================================
Files 37 37
Lines 1786 1786
Branches 1786 1786
==================================================================================================
Hits 1486 1486
Misses 222 222
Partials 78 78 ☔ View full report in Codecov by Sentry. |
7ed8fcc
to
5371054
Compare
c5b88b9
to
e90209c
Compare
e90209c
to
1bedd80
Compare
5371054
to
63f19d2
Compare
1bedd80
to
6f5765a
Compare
63f19d2
to
f00b4a3
Compare
commit-id:d3030c87
f00b4a3
to
6659308
Compare
39ff4d5
to
aa231df
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.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @elintul and @Itay-Tsabary-Starkware)
crates/mempool_infra/src/component_server.rs
line 25 at r2 (raw file):
/// specified component. It receives requests, processes them using the provided component, and /// sends back responses. The server needs to be started using the `start` function, which should be /// invoked in a different task/thread.
I would change it to just saying that 'start' is blocking and not suggesting where/how to invoke it
Code quote:
/// sends back responses. The server needs to be started using the `start` function, which should be
/// invoked in a different task/thread.
crates/mempool_infra/src/component_server.rs
line 68 at r2 (raw file):
/// Ok(()) /// } /// }
This could be confusing, as having a start function is not a requirement for a component. I would prefer to see an example API function, such as do_something(&mut self)
, instead, and that the handle_request
will call or remove it entirely
Code quote:
/// #[async_trait]
/// impl ComponentStarter for MyComponent {
/// async fn start(&mut self) -> Result<(), ComponentStartError> {
/// Ok(())
/// }
/// }
commit-id:a2509b74
aa231df
to
ddcef77
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.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @elintul and @uriel-starkware)
crates/mempool_infra/src/component_server.rs
line 25 at r2 (raw file):
Previously, uriel-starkware wrote…
I would change it to just saying that 'start' is blocking and not suggesting where/how to invoke it
Slightly changed to address the concern.
crates/mempool_infra/src/component_server.rs
line 68 at r2 (raw file):
Previously, uriel-starkware wrote…
This could be confusing, as having a start function is not a requirement for a component. I would prefer to see an example API function, such as
do_something(&mut self)
, instead, and that thehandle_request
will call or remove it entirely
It actually is a requirement.
I added a usage example with a request and a response nonetheless.
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.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @elintul and @Itay-Tsabary-Starkware)
crates/mempool_infra/src/component_server.rs
line 68 at r2 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
It actually is a requirement.
I added a usage example with a request and a response nonetheless.
oh oops I missed it
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.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul and @Itay-Tsabary-Starkware)
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.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @elintul)
commit-id:a2509b74
Stack:
This change is