-
Notifications
You must be signed in to change notification settings - Fork 35
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
Sketch entrypoint for Restate application #8
Conversation
src/restate/src/app.rs
Outdated
pub(crate) struct Application { | ||
meta_service: Meta, | ||
} | ||
|
||
impl Options { | ||
pub(crate) fn build(self) -> Application { | ||
let meta_service = self.meta_options.build(); | ||
|
||
Application { meta_service } | ||
} | ||
} | ||
|
||
impl Application { | ||
pub(crate) fn run(self) -> drain::Signal { | ||
let (signal, _) = drain::channel(); | ||
let (signal, watch) = drain::channel(); | ||
|
||
tokio::spawn(self.meta_service.run(watch)); | ||
|
||
signal | ||
} | ||
} |
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.
Comment you can ignore now: It feels to me this function will grow badly like the Worker
struct in the prototype. I wonder if we can use a different approach, such as not creating at all this Application
struct, but rather have a tuple/vec of N components for which we implement a trait Runnable
, Component
, or something like that, implementing the run
function over a tuple/vec of components. Something like:
// This trait is implemented by Meta, Consensus, Invoker, whatever, ...
trait Component {
pub async fn run(self, drain: drain::Watch) -> Result<(), anyhow::Error>;
}
impl Component for Vec<Box<dyn Component>> {
pub async fn run(self, drain: drain::Watch) -> Result<(), anyhow::Error>;
}
let application = Vec::new(...)
application.run()
This approach allows to pick the components we want to run just by changing the type of this tuple, simplifying creating different "main" bundles (e.g. if and when we'll want to ship separate binaries for meta and worker) and testing (by swapping components with mocks).
Thing is, there still needs to be some logic to wire up components (e.g. channels), so i guess these will be explicited at creation time of the component? At the end of the day, I'm not sure this approach is better until we try...
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.
I think the main complexity comes from wiring the different components up and this is something that we have to do in any case. I think this is also the main problem of the Worker
impl in the POC.
If we see that the Application
contains logic that we want to share between different binaries, then having something like you proposed could be a good generalization. One question I would have is what costs polling a Box<dyn Component>
entails. Would this be a dynamic dispatch for every poll on the dyn Component
(might be premature optimization thinking here)?
For the time being I would suggest to continue simple and stupid and if we see that the number of components becomes unbearable easier to manage in a generic fashion then we'll change 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.
Would this be a dynamic dispatch for every poll on the dyn Component (might be premature optimization thinking here)?
My understanding is that you pay the price once on the dyn Component::run
invocation, and once on the future polling (because it has to be dyn box as well)
The entrypoint parses command line options, instruments the process with logging and tracing, starts the application and waits for a termination signal to shutdown the process.
This commit shows how a meta component can be integrated with the restate entrypoint.
The TryProcessAbortFuture trait allows to abort a process if the TryFuture completed with an error.
The clap options are now better organized by giving options that belong to the same domain the same prefix.
I think all of the discussions are resolved by now. Moving forward with merging this PR now to unblock other efforts. |
This PR sketches the entrypoint for the Restate application:
tracing
crateSIGTERM
andSIGINT
signals for interruptionThis PR resolves #7 and is based on the PR #6.