Service builder refactoring #6309
Conversation
gavofyork
added
the
A3-in_progress
|
I still have to tidy up the node cli/node template service code a little but otherwise I think that this is ready for review now. |
| /// Use this macro if you don't actually need the full service, but just the builder in order to | ||
| /// be able to perform chain operations. | ||
| macro_rules! new_full_start { | ||
| ($config:expr) => {{ |
There was a problem hiding this comment.
It exists because it's used both in new_full! and command::run. I could turn it into a function but the return type would be very complex.
| .build()?; | ||
| let provider = client.clone() as Arc<dyn StorageAndProofProvider<_, _>>; | ||
| let finality_proof_provider = Arc::new(GrandpaFinalityProofProvider::new(backend.clone(), provider)); | ||
| let service = sc_service::build(sc_service::ServiceDescriptor { |
There was a problem hiding this comment.
Essentially we have replaced the builder with a almost 20 lines initialization of a struct. Where is the improvement?
So I know that the current builder is shit, but if we do it now, we should do it correct and not introduce another "intermediate state".
There was a problem hiding this comment.
This resolves a lot of the issues @gnunicorn talks about here: #4587 (comment).
expenses
added
A0-please_review
|
I expect that this pr brings a real benefit to the old code and we not end with an intermediate solution again like last time. This means, we need to think about how to make it better. The current approach does not makes it better, as we replace the builder with this 20 lines initialization code. And than we also have this macro. Please, I hate this macro. It just makes it way more harder. |
|
Since I worked on this I think I understand the point of view of you both. I will summarize in hope it will help: First of all: the original requirements are defined in #4587. The goal is to resolve issues coming from the ServiceBuilder. After analyzing it I realized that the ServiceBuilder itself brings no value (it doesn't help much to build the Service). Removing entirely the ServiceBuilder and replacing it with a few functions already remove a lot of the issues. Second: the real issue with the ServiceBuilder is the way we have to write the code to build the light and the full client. This is not part of #4587 but clearly the pain point is there when you see the The problem is: the "remove & replace" of the first point equals refactoring and does not include the issue of the second point. I personally don't have any idea (at the moment) of an API that would improve the second point. To me it was already a good complicated step to do the changes done already in this PR. A better API could be included in this PR but it means it will be more complicated to review and we get into the feature-branch kind of issue. But maybe it's worth giving a try. |
|
This needs to be solved by experimenting with the API and trying something. It is maybe not that easy, but we don't need to merge something that brings no value. It may looks easier on the first glance, in the end it has the same problems as the old solution. For me it is fine to go the way just have a free function that will initialize all the components and stick them together. I would first start with moving the initialization code per component into the crate of the component and not having it in the service build function itself. The component knows what it needs and how it needs to be initialized. In the end we probably only need the client + the task manager. Because most of the stuff is spawned as a future anyway and is not part of the service anymore. If we go this road, we probably can remove the |
|
@bkchr Ok, I'll look into that. |
|
Ty |
|
I'm going to close this PR for the time being whilst I work on refactoring initialization code. |
4 participants
This is a continuation of the Service builder refactoring that @cecton started in #5557.