Use promises in the ExecutionRecorder interface #4186
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4186 +/- ##
==========================================
+ Coverage 32.94% 33.16% +0.22%
==========================================
Files 459 459
Lines 55830 55832 +2
==========================================
+ Hits 18394 18519 +125
+ Misses 34212 34075 -137
- Partials 3224 3238 +14 |
❌ 5 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
bragaigor
left a comment
bragaigor
left a comment
There was a problem hiding this comment.
Do we still need to implement ExecutionRecorder methods for ExecutionRPCServer? Something like:
func (c *ExecutionRPCServer) RecordBlockCreation(
ctx context.Context,
pos arbutil.MessageIndex,
msg *arbostypes.MessageWithMetadata,
wasmTargets []rawdb.WasmTarget,
) (*execution.RecordResult, error) {
return c.executionRecorder.RecordBlockCreation(ctx, pos, msg, wasmTargets).Await(ctx)
}and same for PrepareForRecord? But then we would need to add executionRecorder to ExecutionRPCServer or modify ExecutionClient. Or that's be part of another PR? CC: @diegoximenes
|
@bragaigor yes, definitely, but I didn't want to do it in the same PR to keep changes short and granular; this PR is just a part of NIT-4063 (see the description) |
|
@pmikolajczyk41 got it, apologies I missread the description. Also prefers smaller increment PRs |
| prevHeader = r.execEngine.bc.GetHeaderByNumber(uint64(blockNum - 1)) | ||
| if prevHeader == nil { | ||
| return nil, fmt.Errorf("pos %d prevHeader not found", pos) | ||
| ) containers.PromiseInterface[*execution.RecordResult] { |
There was a problem hiding this comment.
Instead of calling stopwaiter.LaunchPromiseThread here you can call in ExecutionNode.RecordBlockCreation.
This will follow the same pattern that is deployed by ExecutionNode/ExecutionClient today, such as being deployed by ExecutionNode.ShouldTriggerMaintenance.
And it will also decrease code diff in this PR.
| if prevHeader == nil { | ||
| return nil, fmt.Errorf("pos %d prevHeader not found", pos) | ||
| ) containers.PromiseInterface[*execution.RecordResult] { | ||
| return stopwaiter.LaunchPromiseThread(r.execEngine, func(ctx context.Context) (*execution.RecordResult, error) { |
There was a problem hiding this comment.
Nothing to do about this right now, this is more a question to @tsahee when he reviews this PR 🙂.
Since Promises/LaunchPromiseThread/etc are not a golang native way to handle concurrency, it is not clear to me what would be the ideal scenario here.
It is not clear to me why StopWaiter.LaunchPromiseThread accepts a ThreadLauncher (a StopWaiter "implements" a ThreadLauncher), while StopWaiter.LaunchThread doesn't, for example.
Here ExecutionEngine's StopWaiter is being used to launch a thread in BlockRecorder, and the ctx related to this thread is being used to interact with RecordingDatabase.
Not sure if there is an issue with this.
Make
execution.ExecutionRecorderinterface use promise API:ExecutionClientinterfacepart of NIT-4063