Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Introduce RPC Worker #1462

Merged
merged 19 commits into from
Aug 1, 2022
Merged

Introduce RPC Worker #1462

merged 19 commits into from
Aug 1, 2022

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Jul 21, 2022

This PR is the next step towards #989.

Currently the collator opens a new RPC subscription every time we request a notification stream from the RelayChainRPCInterface.

After this PR is merged, we will have only three open notification subscriptionsvia RPC:

  • finalized block headers
  • imported block headers
  • new best block headers

A worker is introduced that will poll these three subscriptions regularly and distribute the contained headers to all listening streams. Requesting a stream from the RelayChainRPCInterface will return a new channel and inform the worker.
Since the streams are only used in the collator, and we have complete control, we use bounded channels with a limit of 20 items.

@skunert skunert added A0-pleasereview B0-silent Changes should not be mentioned in any release notes labels Jul 21, 2022
@skunert skunert requested a review from a team July 21, 2022 13:00
@skunert skunert marked this pull request as ready for review July 21, 2022 13:24
@davxy
Copy link
Member

davxy commented Jul 27, 2022

Really silly nit. Maybe we can stick to the notation where also the acronyms are usually written in camel case.
In other words RPC -> Rpc. Thus, for example rename RelayChainRPCClient to RelayChainRpcClient

Feel free to ignore me :-D

Copy link
Member

@davxy davxy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I've just added a suggestion to set the RpcStreamWorker as private

client/relay-chain-rpc-interface/src/lib.rs Outdated Show resolved Hide resolved
client/relay-chain-rpc-interface/src/rpc_client.rs Outdated Show resolved Hide resolved
Co-authored-by: Davide Galassi <[email protected]>
Copy link
Contributor

@koute koute left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unbounded channel's still making me a little nervous, but I'll leave the decision as to what to do with it up to you. Otherwise LGTM.

@skunert skunert merged commit b6a2a38 into paritytech:master Aug 1, 2022
jiguantong added a commit to darwinia-network/darwinia-parachain that referenced this pull request Nov 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants