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

Simplify subsystem jobs #2037

Merged
merged 3 commits into from
Nov 30, 2020
Merged

Simplify subsystem jobs #2037

merged 3 commits into from
Nov 30, 2020

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Nov 30, 2020

This pr simplifies the subsystem jobs interface. Instead of requiring an
extra message that is used to signal that a job should be ended, a job
now ends when the receiver returns None. Besides that it changes the
interface to enforce that messages to a job provide a relay parent.

This pr simplifies the subsystem jobs interface. Instead of requiring an
extra message that is used to signal that a job should be ended, a job
now ends when the receiver returns `None`. Besides that it changes the
interface to enforce that messages to a job provide a relay parent.
@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Nov 30, 2020
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Agree this does what it says. It could go farther: getting rid of ToJob would enable us to remove even more code.

/// If the message variant contains its relay parent, return it here
fn relay_parent(&self) -> Option<Hash>;
/// Get the relay parent this message is assigned to.
fn relay_parent(&self) -> Hash;
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes ToJobTrait a bit more rigid: it ensures that we'll never write a subsystem using this util which can have jobs not tied to relay parents.

I think this is a reasonable simplification at this point.

node/subsystem-util/src/lib.rs Outdated Show resolved Hide resolved
node/subsystem/src/messages.rs Outdated Show resolved Hide resolved
We always convert this message to FromJobCommand anyway.
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Nice 👍

@ordian ordian merged commit 1fbf09a into master Nov 30, 2020
@ordian ordian deleted the bkchr-simplify-subsystem-jobs branch November 30, 2020 16:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants