Initialize using Dependency Set Configuration #12495
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…, improve backend resource initialization
protolambda
force-pushed
the
interop/use-dependency-set
branch
from
88437f8 to
b849ab4
Compare
axelKingsley
commented
Oct 17, 2024
Contributor
Author
axelKingsley
left a comment
axelKingsley
left a comment
There was a problem hiding this comment.
I'm the author of this PR so I can't approve it, but since @protolambda added some things, I'm going to review/annotate things to make review as easy as possible for @tyler-smith.
@protolambda generally in favor of the changes you added.
- I see you removed
checkProcessorCoverage, first taking out the error return, then removing the function altogether. I think that function is pretty helpful because it can signal to callers that the backend isn't properly initialized yet, and the log messages would be very nice to see if ever a supervisor is failing due to incomplete coverage. - Looks like you put together a different synchronization mechanism for the Processors. That's fine, but I'm not sure it really belonged in this PR.
I would approve this if I wasn't the author :)
Co-authored-by: Axel Kingsley <axel.kingsley@gmail.com>
protolambda
approved these changes
Oct 17, 2024
samlaf
pushed a commit
to samlaf/optimism
that referenced
this pull request
Nov 10, 2024
* Initialize using Dependency Set Configuration * op-supervisor: init fromda, route fromda metrics, handle cross-unsafe, improve backend resource initialization * op-supervisor: attach RPC, create processors upfront, implement backend test * op-supervisor: fix dependency set configuration and test setup * Update op-supervisor/supervisor/backend/backend.go Co-authored-by: Axel Kingsley <axel.kingsley@gmail.com> --------- Co-authored-by: protolambda <proto@protolambda.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Uses the new Dependency Set Configuration to spin up database constructs for exactly the chains specified.
How
I split out the "AddL2FromRPC" functionality into two parts:
openChainDBs
Creates the LogDB and FromDA DBs for the given chain and attaches it to the
SupervisorBackend.chainsDB.addProcessorFromRPCattachRPCConnects via RPC to determine ChainID, and then attaches a new processor to
SupervisorBackend.ChainProcessors.If the discovered ChainID is not part of the dependency set (determined by known processors), this attachment fails.
Chain-Processors are now instantiated upfront, to allow attaching of RPCs during runtime, and to make the test-setup a little more sane (we can attach a mock source with
AttachProcessorSourcenow)Additionally
The
SupervisorBackendnow tracks a map forChainMetrics, as both the Processor and Database want to use it, and the processor may not be attached until later.Now, when users use the
AddRPCfunction on the Supervisor, it only follows theaddProcessorFromRPCpath.And a minor bugfix in the
logsdb, to read the full block seal properly.TODO:
Unit Tests(done: there's a backend test asserting the presence of DBs and unsafe/cross-unsafe block functionality)Creating the(done)fromdaobjects inside theChainsDBwhen the call is madeMetadata
Fix #12487