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

CHANNEL_CAPACITY is unused #28

Closed
drahnr opened this issue Jan 27, 2023 · 9 comments · Fixed by #31
Closed

CHANNEL_CAPACITY is unused #28

drahnr opened this issue Jan 27, 2023 · 9 comments · Fixed by #31
Assignees

Comments

@drahnr
Copy link
Collaborator

drahnr commented Jan 27, 2023

Since 0.0.5 I get the warning message CHANNEL_CAPACITY from generated code.

@vstakhov
Copy link
Contributor

Indeed, this constant is no longer used as we use it in the builder implementation.

@vstakhov
Copy link
Contributor

In the meantime, we also have signal_channel_capacity, is there any case if we want to have it per-subsystem specific? If yes, then we can unify all the code.

@drahnr
Copy link
Collaborator Author

drahnr commented Jan 27, 2023

The signal capacity is identical per subsystem, so we should never come to the point that we need different queues, unless there is a subsystem that blocks on tasks for a very long time and does not process signals.

vstakhov added a commit that referenced this issue Jan 27, 2023
@vstakhov
Copy link
Contributor

Yes, but if we can override message capacity per subsystem, why cannot we override a signal capacity then?

@drahnr
Copy link
Collaborator Author

drahnr commented Jan 27, 2023

Signals should be homogenous across all subsystems, and should be processed with priority over messages iirc, under that assumption I'd consider it a bug in the Subsystem if its blocking incoming signal consumption for too long

vstakhov added a commit that referenced this issue Jan 27, 2023
@vstakhov
Copy link
Contributor

Yes, but some subsystems might be slow, so signals sent to them might block other susbsystems, similarly to paritytech/polkadot#18.

@drahnr
Copy link
Collaborator Author

drahnr commented Jan 27, 2023

This taps into paritytech/polkadot-sdk#933 whether that's ok or not or if we should split Signal Processing into its own stream entirely

@drahnr
Copy link
Collaborator Author

drahnr commented Jan 27, 2023

But I see that that'd be a large refactor mostly in polkadot, which might not be desirable at this point. Maybe it could be done by introducing prioritization into prioritized-metered-channel. But I degress.

The proposed approach is fine by me, but we should slow down the duck tape fixes needed short term for some more principal led arch refactors

@vstakhov
Copy link
Contributor

In fact, I also think that having both message_capacity and signal_capacity on per-subsystem and per-overseer levels is a more consistent approach.

vstakhov added a commit that referenced this issue Feb 2, 2023
@drahnr drahnr closed this as completed in #31 Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants