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.07% -0.02%
==========================================
Files 2121 2121
Lines 185912 185880 -32
==========================================
- Hits 76395 76345 -50
- Misses 103104 103117 +13
- Partials 6413 6418 +5
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.
| @@ -57,6 +58,8 @@ func (s *MatchingEngineSuite) SetupTest() { | |||
| s.engine, err = NewEngine(unittest.Logger(), net, me, metrics, metrics, s.state, s.receipts, s.index, s.core) | |||
| require.NoError(s.T(), err) | |||
|
|
|||
| ctx := irrecoverable.NewMockSignalerContext(s.T(), context.Background()) | |||
There was a problem hiding this comment.
Could you also add teardown logic to make sure we are gracefully stopping the engine?
- Use
NewMockSignalerContextWithCanceland store theCancelFuncin the test suite struct - Add a
TearDownTestfunction, callcancel()and wait forengine.DoneusingAssertClosesBefore
| @@ -57,6 +58,8 @@ func (s *MatchingEngineSuite) SetupTest() { | |||
| s.engine, err = NewEngine(unittest.Logger(), net, me, metrics, metrics, s.state, s.receipts, s.index, s.core) | |||
| require.NoError(s.T(), err) | |||
|
|
|||
| ctx := irrecoverable.NewMockSignalerContext(s.T(), context.Background()) | |||
| s.engine.Start(ctx) | |||
| <-s.engine.Ready() | |||
There was a problem hiding this comment.
Could you wrap this in AssertClosesBefore
Refactor the Consensus Matching Engine to move away from deprecated interfaces, as well as improve error handling and documentation. See issue #6854. 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 contexts