Refactor Consensus Matching Engine: engine.Unit -> ComponentManager
#6916
Conversation
…Processor Remove `Submit`, `SubmitLocal`, `ProcessLocal` that implemented network.Engine, clean up error checking, and update doc comments
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6916 +/- ##
=======================================
Coverage 41.09% 41.10%
=======================================
Files 2121 2119 -2
Lines 185912 185867 -45
=======================================
Hits 76395 76395
+ Misses 103104 103066 -38
+ Partials 6413 6406 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| e.log.Fatal().Err(err).Msg("internal error processing event from requester module") | ||
| r, ok := receipt.(*flow.ExecutionReceipt) | ||
| if !ok { | ||
| e.log.Fatal().Err(engine.IncompatibleInputTypeError).Msg("internal error processing event from requester module") |
There was a problem hiding this comment.
I think that ideally HandleReceipt would also be able to use ctx.Throw instead of Log.Fatal, but the function type is already dictated by being used as a HandleFunc by the Requester engine. Could be pushed to a future refactor of Requester Engine (since that one also still uses engine.Unit).
There was a problem hiding this comment.
I agree that this could be done as part of updating the requester engine. It would be great to have type-safe handler functions in the requester engine, which we could implement by making the requester engine and its Create/Handle functions generic.
| e.log.Fatal().Err(err).Msg("internal error processing event from requester module") | ||
| r, ok := receipt.(*flow.ExecutionReceipt) | ||
| if !ok { | ||
| e.log.Fatal().Err(engine.IncompatibleInputTypeError).Msg("internal error processing event from requester module") |
There was a problem hiding this comment.
I agree that this could be done as part of updating the requester engine. It would be great to have type-safe handler functions in the requester engine, which we could implement by making the requester engine and its Create/Handle functions generic.
There was a problem hiding this comment.
I think it is an important best practise to register the component with the networking layer (👉 this code) only at the very end of the constructor, after we have fully constructed the component (including all the worker routines 👉 this code).
I like to frame this as a very foundational best practise in software engineering: don't reveal a reference to a component until the component is fully constructed. Usually, we implement this via a constructor that only at the end returns the reference to the constructed object. Our business logic is a bit more complicated, in that we don't want to rely on the caller to registers the matching.Engine with the networking layer - so we do it within the constructor; however then we have to make sure we are not revealing the pointer of the object under construction until the object instantiation is properly completed.
There was a problem hiding this comment.
That makes sense - I wasn't thinking deliberately enough about network interfaces, and was assuming the ComponentManager would want to be last due to the workers it starts potentially needing to access parts of the engine under construction for a similar reason; but basically forgot that the worker routines will only actually need those resources once the component is Started (after construction).
Remove separate ComponentManager pointer since only the Component interface is used Co-authored-by: Alexander Hentschel <[email protected]>
Ensure the engine can only be accessed externally once construction is complete. See #6916 (comment)
tim-barry
deleted the
tim/6854-consensus-matching-engine-componentmanager-refactor
branch
Refactor the Consensus Matching Engine to move away from deprecated interfaces, as well as improve error handling and documentation. The changes are relatively straightforward drop-in replacements, due to the engine already being structured as an initial set of workers launched when the engine is started.
engine.UnitwithComponentManagernetwork.Enginewithnetwork.MessageProcessorctx.Throwto propagate irrecoverable errors instead ofLog.Fatal, which also allows for tests to verify behaviour by using mock contextsCloses #6854