Service factory refactor #3382

tomaka · 2019-08-13T12:43:07Z

General overview of the changes:

Removes the components.rs module and almost everything that was in it.
Adds a ServiceBuilder struct.
Removes construct_service_factory!. You are now supposed to define your own functions (eg. new_full and new_light) that create the service using the ServiceBuilder.
Adds a lot of generics to Service and adds an AbstractService trait that you are expected to use in practice when you pass the Service around.

I started with very clean and detailed commits, but since I keep getting conflicts with master, and that rebasing is very painful, I rushed the end a bit more. I'd still recommend to review commit by commit.

All commits up to and including Fix the node-cli tests don't break the API. Only the commits afterwards are breaking changes. I would suggest to merge that PR in two parts: up to that commit, then the rest, so that we can revert "the rest" if the refactoring blocks Polkadot. This is because updating Polkadot will not be trivial, and I don't want to take the blame for delaying Kusama because this PR was merged too eagerly. I opened #3384 with the first part separately.

~~Remains to be done before merging:~~

~~- Restore the sync test of node-cli. This test is the one victim of this refactoring, and I need a bit more time figuring out how to restore it.~~

node-template/src/service.rs

mxinden

Low priority suggestion: Given that core/service/src/factory.rs contains the ServiceBuilder logic and given that ServiceBuilder follows the builder pattern and not the factory pattern, how about renaming factory.rs to builder.rs?

tomaka · 2019-08-15T14:17:23Z

As mentioned in the other PR, I can continue piling up changes, but there's an arbitrary point where I have to stop and submit it.

I agree with the rename, but I also don't really want to continue pushing commits that don't address issues in the PR.

bkchr

The general structure goes into the right direction, but I don't understand why we have all these new macros. We wanted to get rid of the macro and not introduce multiple new ones.

For the builder, I would like to see a function with_authority_task or a similar name. This should be used by Babe or Aura, so you don't need to check yourself if the current node is an authority.

bkchr · 2019-08-13T13:15:31Z

core/cli/src/informant.rs

-pub fn start<C>(service: &Service<C>, exit: ::exit_future::Exit, handle: TaskExecutor) where
-	C: Components,
-{
+pub fn start(service: &impl AbstractService, exit: ::exit_future::Exit, handle: TaskExecutor) {


Can we not finally remove this function?

bkchr · 2019-08-13T13:21:59Z

core/finality-grandpa/Cargo.toml

@@ -23,7 +23,7 @@ serde_json = "1.0"
 client = { package = "substrate-client", path = "../client" }
 inherents = { package = "substrate-inherents", path = "../../core/inherents" }
 network = { package = "substrate-network", path = "../network" }
-service = { package = "substrate-service", path = "../service", optional = true }
+service = { package = "substrate-service", path = "../service" }


This import seems to be used only for import TelemetryOnConnect. Isn't that just a stream that tells us when we reconnected to the telemetry? Can we not define this locally?

test-utils/transaction-factory/src/simple_modes.rs

bkchr · 2019-08-14T12:31:14Z

node-template/src/service.rs

+///
+/// 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 {


I don't understand the reason for this macro. Can we not put all this stuff into a function?

The reason why this macro exists is that we don't want to build a full service when for example importing blocks. In order to import blocks, we only need the client, select-chain and import-queue.

I'm using a macro rather than a function because the return type would be very verbose. This is node-template and is supposed to be tweakable for the user, so anything that looks complicated is negative.

Maybe we can introduce a ServiceIntermediaryType trait in the future, or something like that. Since the design needs to be figured out, I'd like to delay that for further refactoring, if that's ok.

bkchr · 2019-08-14T12:48:11Z

core/service/src/lib.rs

+	/// Backend storage for the client.
+	type Backend: 'static + client::backend::Backend<Self::Block, Blake2Hasher>;
+	/// How to execute calls towards the runtime.
+	type Executor: 'static + client::CallExecutor<Self::Block, Blake2Hasher> + Send + Sync + Clone;


Suggested change

type Executor: 'static + client::CallExecutor<Self::Block, Blake2Hasher> + Send + Sync + Clone;

type CallExecutor: 'static + client::CallExecutor<Self::Block, Blake2Hasher> + Send + Sync + Clone;

Maybe? To not mistake it with future::Executor?

bkchr · 2019-08-14T12:48:24Z

core/service/src/lib.rs

@@ -617,6 +665,80 @@ impl<Components> Executor<Box<dyn Future<Item = (), Error = ()> + Send>>
 	}
 }

+impl<T> AbstractService for T


Why do we need this impl?

This was in order to implement AbstractService on FullComponents and LightComponents. Will remove.

node/cli/src/service.rs

tomaka · 2019-08-19T08:42:29Z

I don't understand why we have all these new macros. We wanted to get rid of the macro and not introduce multiple new ones.

If you're talking about the macros in node, one is an unfortunate consequence of the fact that some of the Substrate tests are using the node. Both macros can probably be removed after more future refactoring.

If you're talking about the macros within substrate-service, they are purely internal and similarly I'm going to remove them in the future.

As mentioned, there are tons of changes to make (including the ones you raised in your review), but I didn't want to have a PR that conflicts with half of Substrate.

bkchr · 2019-08-21T14:23:48Z

Can you maybe at least do the basic review comments that do not touch a macro? If you say to all comments "I don't want to push any other commit", I don't see any reason at all why someone should review this 🤷‍♂️
I understand that you want to get this in relative fast, to reduce the number of conflicts. But then please at least address the obvious stuff, like the RA, RtApi generic arguments, the incorrect named file etc. After that we can merge, if we see the final version in the next pr. I also have some ideas and requirements and don't want to make sure I don't miss something across multiple prs.
Otherwise the service stuff will probably calm down after we have Kusama released.

In follow-up commits, we want to be able to directly call maintain_transaction_pool, offchain_workers, and start_rpc, without having to implement the Components trait. This commit is a preliminary step: we extract the code to freestanding functions.

Instead of implementing AbstractService, Future, and Executor on Service, we implement them on NewService instead. The implementations of AbstractService, Future, and Executor on Service still exist, but they just wrap to the respective implementations for NewService.

Instead of having multiple $build_ parameters passed to the macro, let's group them all into one. This change is necessary for the follow-up commits, because we are going to call new_impl! only after all the components have already been built.

This makes it possible to be explicit as what the generic parameter of the NewServiceis, without relying on type inference.

Introduces a new builder-like ServiceBuilder struct that creates a NewService.

Similar to the introduction of new_impl!, we extract the actual code into a macro, letting us get rid of the Components and Factory traits

…uilder Can be used as a replacement for the chain_ops::* methods

Instead of just run, adds run_with_builder to ParseAndPrepareExport/Import/Revert. This lets you run these operations with a ServiceBuilder instead of a ServiceFactory.

This is technically a breaking change, but the transaction-factory crate is only ever used from within substrate-node, which this commit updates as well.

We slightly change the trait bounds in order to make all the methods usable.

Co-Authored-By: Gavin Wood <[email protected]>

tomaka · 2019-08-26T12:57:15Z

Should be ready for review/merging. I fixed most of the remarks, and would like to delay the ones that aren't trivial to fix in order to get this in sooner.

tomaka mentioned this pull request Aug 13, 2019

Service factory refactor, part 1 #3384

Closed

tomaka added the A0-please_review Pull request needs code review. label Aug 13, 2019

tomaka requested a review from bkchr August 13, 2019 13:06

gavofyork reviewed Aug 13, 2019

View reviewed changes

node-template/src/service.rs Outdated Show resolved Hide resolved

gavofyork reviewed Aug 13, 2019

View reviewed changes

node-template/src/service.rs Outdated Show resolved Hide resolved

tomaka added the B2-breaksapi label Aug 13, 2019

mxinden reviewed Aug 15, 2019

View reviewed changes

bkchr suggested changes Aug 19, 2019

View reviewed changes

mxinden mentioned this pull request Aug 21, 2019

core/authority-discovery: Enable authorities to discover each other #3452

Merged

svyatonik mentioned this pull request Aug 26, 2019

Make chain && state RPCs async #3480

Merged

3 tasks

tomaka added 17 commits August 26, 2019 11:02

Move Service::new to a macro

9fda259

Move function calls to macros

ec9c66d

Introduce an AbstractService trait

7600276

Introduce NewService as an implementation detail of Service

4af1fc7

Add a $block parameter to new_impl

e169901

This makes it possible to be explicit as what the generic parameter of the NewServiceis, without relying on type inference.

Introduce the ServiceBuilder struct

9d667d8

Introduces a new builder-like ServiceBuilder struct that creates a NewService.

Macro-ify import_blocks, export_blocks and revert_chain

4fc8aac

Similar to the introduction of new_impl!, we extract the actual code into a macro, letting us get rid of the Components and Factory traits

Add export_blocks, import_blocks and revert_chain methods on ServiceB…

8467efb

…uilder Can be used as a replacement for the chain_ops::* methods

Add run_with_builder

be2a712

Instead of just run, adds run_with_builder to ParseAndPrepareExport/Import/Revert. This lets you run these operations with a ServiceBuilder instead of a ServiceFactory.

Transition node and node-template to ServiceBuilder

7eee475

Transition transaction-factory to the new service factory

8e623c7

This is technically a breaking change, but the transaction-factory crate is only ever used from within substrate-node, which this commit updates as well.

Remove old service factory

3d8f9cc

Adjust the AbstractService trait to be more usable

f290efe

We slightly change the trait bounds in order to make all the methods usable.

Make substrate-service-test compile

8107b28

tomaka and others added 9 commits August 26, 2019 13:17

Fix the node-cli tests

a44e91f

Remove the old API

00e5e35

Remove the components module

98252b4

Fix indentation on chain_ops

1bded02

Line widths

bd34a60

Fix bad line widths commit

0ebb4fe

Line widths again 🤦

7f4c953

Fix the sync test

1dbe6f6

Apply suggestions from code review

69f3131

Co-Authored-By: Gavin Wood <[email protected]>

tomaka force-pushed the service-factory-refactor branch from e7eb504 to 69f3131 Compare August 26, 2019 12:07

tomaka added 5 commits August 26, 2019 14:30

Address some concerns

f814f4e

Remove TelemetryOnConnect

36d3f27

Remove informant::start

9b034c4

Update jsonrpc

df96677

Rename factory to builder

1695b73

Line widths 😩

9374085

bkchr approved these changes Aug 27, 2019

View reviewed changes

bkchr merged commit d14e727 into paritytech:master Aug 27, 2019

tomaka deleted the service-factory-refactor branch August 27, 2019 09:20

tomaka mentioned this pull request Aug 27, 2019

Update to latest Substrate paritytech/polkadot#407

Merged

andresilva mentioned this pull request Aug 28, 2019

node: spawn babe epoch pruning in light client #3505

Merged

andresilva mentioned this pull request Oct 23, 2019

node: spawn grandpa voter as essential task #3899

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service factory refactor #3382

Service factory refactor #3382

tomaka commented Aug 13, 2019 •

edited

Loading

mxinden left a comment

tomaka commented Aug 15, 2019 •

edited

Loading

bkchr left a comment

bkchr Aug 13, 2019

bkchr Aug 13, 2019

bkchr Aug 14, 2019

tomaka Aug 26, 2019

bkchr Aug 14, 2019

bkchr Aug 14, 2019

tomaka Aug 26, 2019

tomaka commented Aug 19, 2019

bkchr commented Aug 21, 2019

tomaka commented Aug 26, 2019

	type Executor: 'static + client::CallExecutor<Self::Block, Blake2Hasher> + Send + Sync + Clone;
	type CallExecutor: 'static + client::CallExecutor<Self::Block, Blake2Hasher> + Send + Sync + Clone;

Service factory refactor #3382

Service factory refactor #3382

Conversation

tomaka commented Aug 13, 2019 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

tomaka commented Aug 15, 2019 • edited Loading

bkchr left a comment

Choose a reason for hiding this comment

bkchr Aug 13, 2019

Choose a reason for hiding this comment

bkchr Aug 13, 2019

Choose a reason for hiding this comment

bkchr Aug 14, 2019

Choose a reason for hiding this comment

tomaka Aug 26, 2019

Choose a reason for hiding this comment

bkchr Aug 14, 2019

Choose a reason for hiding this comment

bkchr Aug 14, 2019

Choose a reason for hiding this comment

tomaka Aug 26, 2019

Choose a reason for hiding this comment

tomaka commented Aug 19, 2019

bkchr commented Aug 21, 2019

tomaka commented Aug 26, 2019

tomaka commented Aug 13, 2019 •

edited

Loading

tomaka commented Aug 15, 2019 •

edited

Loading