Skip to content
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

Make node syncing messages configurable. #2110

Closed
2 tasks done
shamil-gadelshin opened this issue Nov 1, 2023 · 4 comments · Fixed by #3166
Closed
2 tasks done

Make node syncing messages configurable. #2110

shamil-gadelshin opened this issue Nov 1, 2023 · 4 comments · Fixed by #3166
Labels
I5-enhancement An additional feature request. I10-unconfirmed Issue might be valid, but it's not yet known.

Comments

@shamil-gadelshin
Copy link
Contributor

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Motivation

Currently, we have node syncing messages produced by the InformantDisplay from sc-informant crate within sc-service. There is no way to alter the messages or change their periodicity which could be suboptimal for some parachains.

Request

I suggest making InformantDisplay configurable in the sc_service crate.

Solution

Make SpawnTasksParams from sc_service accept an optional additional parameter.

There are several variants for this parameter:

  1. Opaque future similar to the current output of sc_informant::build().
  2. Stream of messages.
  3. Introduce a dedicated trait with the methods providing 1. or 2. but extendable in the future.

Are you willing to help with this request?

Yes!

@shamil-gadelshin shamil-gadelshin added the I5-enhancement An additional feature request. label Nov 1, 2023
@github-actions github-actions bot added the I10-unconfirmed Issue might be valid, but it's not yet known. label Nov 1, 2023
@bkchr
Copy link
Member

bkchr commented Nov 2, 2023

This entire spawn_tasks function is something I don't like. It doesn't bring any real value and we should remove it at some point.

@shamil-gadelshin
Copy link
Contributor Author

Are you suggesting just not to use this function at all and spawn the necessary tasks separately? Or you have something else in mind?

@nazar-pc
Copy link
Contributor

nazar-pc commented Jan 3, 2024

I hit another annoyance with spawn_tasks, it requires sp_session::SessionKeys runtime API, even though it makes no sense for our chain.

The difficulty with spawn_tasks is that while it is fairly small, it relies on a bunch of private functions that anyone reimplementing spawn_tasks would have to replicate too.

What do you think about exposing those functions publicly? I can send a PR.

@bkchr
Copy link
Member

bkchr commented Jan 29, 2024

What do you think about exposing those functions publicly? I can send a PR.

Sounds fine to me.

github-merge-queue bot pushed a commit that referenced this issue Feb 3, 2024
This allows to build a custom version of `spawn_tasks` with less
copy-paste required.

Resolves #2110

---------

Co-authored-by: Bastian Köcher <[email protected]>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
This allows to build a custom version of `spawn_tasks` with less
copy-paste required.

Resolves paritytech#2110

---------

Co-authored-by: Bastian Köcher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. I10-unconfirmed Issue might be valid, but it's not yet known.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants