-
Notifications
You must be signed in to change notification settings - Fork 86
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
refactor(conductor): make firm, soft readers subtasks #1926
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits, but otherwise LGTM. I think this refactor will be very nice to have!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment only.
making the celestia (firm) and astria (soft) readers subtasks of the executor task is a more faithful representation of their dependencies: executor can run with either or both present. But the reader tasks cannot run without the executor task present. To fully initialize they also depend on data from the executor, and they could be implemented by streams instead of free standing tasks. spin up readers only after commitment, genesis states are received this allows removing a lot of complexity: 1. the readers need not explicitly wait for the state to be initialized but receive an already initialized watch channel. 2. there is no need for a bespoke channel to dynamically set permits - a normal mpsc channel can be used with its capacity initialized after receiving the genesis info. executor::Initialized::run delegates to executor::Initialized::run_event_loop to separate the shutdown token from the other arms of the select macro - this way, an else => {} arm can be introduced that shuts down executor as a fallback
7314f8a
to
6a69de9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stamp
Summary
Makes the firm and soft readers subtasks of the main exection loop.
Background
Making the celestia (firm) and astria (soft)
readers subtasks of the executor loop is a more
faithful representation of their dependencies:
execution loop present (the tasks do not run at the
same level of privilege).
data provided via the execution loop (rollup genesis
info and commitment state).
This patch then spins up the readers during initialization
of the execution event loop, which allows removing a lot
of complexity from the conductor codebase:
initialized but receive an already initialized state.
set permits (this was needed for dynamic backpressure
handling by setting the channel capacity after reading the
celestia variance from the rollup): a standard mpsc channel
can be instantiated and then passed to the (soft/sequencer)
reader.
executor::Initialized::run delegates to
executor::Initialized::run_event_loop to separate
the shutdown token from the other arms of the
select macro - this way, an else => {} arm can be
introduced that shuts down executor as a fallback
Changes
JoinMap
from theconductor::Inner
business logic as this isnow tracked by the executor task
from its main event loop so that the
tokio::select!
macro'selse => {}
armcan be used as a fallback.
executor::channel
for dynamically setting channelcapacity at runtime (no longer necessary; the channel is initialized later
after we know its capacity)
State
watch channel notifying the readers of the latest rollupstate by removing the
StateIsInit
andStateNotInit
markers (the readersnow get a fully initialized watch channel).
Testing
These changes should be transparent and not noticeable. The blackbox tests
still pass which implies that the (tested) conductor behavior works.
Changelogs
NOTE: Update before merge
Ensure all relevant changelog files are updated as necessary. See
keepachangelog for change
categories. Replace this text with e.g. "Changelogs updated." or "No updates
required." to acknowledge changelogs have been considered.
Related Issues
NOTE: Link the forma issue?
Link any issues that are related, prefer full GitHub links.
closes