Skip to content
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: Replace some uses of engine.Unit with ComponentManager #6833

Merged
merged 42 commits into from
Dec 21, 2024

Conversation

tim-barry
Copy link
Contributor

@tim-barry tim-barry commented Dec 20, 2024

Only contains changes from previously-reviewed PRs: #6747, #6780, #6808, and #6809, covering the Collection node Pusher engine, Verification node Assigner/Fetcher engines, and Execution node AsyncUploader.
Part of #6807.

tim-barry and others added 30 commits November 20, 2024 16:02
Using #4219 as an example.
Instead of starting new goroutines or directly processing messages
in a blocking way, messages are added to a queue that a worker pulls from.
The Pusher engine still currently implements network.Engine rather than
network.MessageProcessor.
Because the event processing now happens in a worker, any errors
raised within it are no longer visible to the caller of Process().
Because the test checked for error status, moved the tests to the
same package and call the internal processing function directly.
When using Unit, calling Ready would also start the engine.
With ComponentManager, we additionally need to invoke Start.
Rename `inboundMessageWorker` and `processInboundMessages` to `outbound`
and also propagate errors to the top level of the worker
where they can be thrown.
- Partially implement suggestion #6747 (comment)
  - Make `SubmitCollectionGuarantee` non-exported and rename to `publishCollectionGuarantee`
  - Add new `SubmitCollectionGuarantee` exported function that just adds to the queue
- Remove `messageHandler` field, instead directly add to queue
  from review: #6747 (comment)
- `OriginID`s no longer included in messages in the queue, and therefore not
   checked by the worker - if necessary they should be checked when Submitting
This reverts commit 0aadc13.
Instead of testing the internals, test the exported interface.
Rename queue and add length metrics for it, updating creation sites.
Co-authored-by: Alexander Hentschel <[email protected]>
Doc comment changes, metrics naming, and queue length.
For reasoning behind chosen queue length, see #6747 (comment)

Co-authored-by: Jordan Schalm <[email protected]>
Instead of logging an error, add to the metrics when the queue is full
and a message needs to be dropped instead of sent.
https://github.com/onflow/flow-go/blob/85913ad09e605fb5234155301bb0d517946a75a5/engine/collection/compliance/engine.go#L139-L146
…nentmanager

Refactor pusher engine part 1

replace engine.Unit with ComponentManager in Pusher Engine
- Remove pusher engine implementation of network.Engine
  - Replace with network.MessageProcessor
  - See: #6747 (comment)
- Remove SubmitCollectionGuarantee message type
  - Was only used between Finalizer and Pusher engine
  - New interface passes and stores collection guarantees directly,
    instead of wrapping and then unwrapping them
  - See: #6747 (comment)
- Add GuaranteedCollectionPublisher interface, implemented by pusher engine
  - Only used by the Finalizer (and intermediate constructors)
  - Mocks are generated for it, used in Finalizer unit tests
  - See: #6747 (comment)
- Construct the mock objects with their `.New___()` method instead of using
  golang's built-in `new` function, which enables automatic cleanup.
- Replace explicit AssertCalled checks at the end of tests with .On() and .Once()
  which will automatically be checked at the end of the test.
Improve documentation for Process method of pusher engine,
and log an error instead of returning an error.
See: #6780 (comment)

Co-authored-by: Alexander Hentschel <[email protected]>
…terface

Refactor Pusher Engine (part 2) - updated interface
The engine.Unit was only used for its context. In similar situations, other code
uses a context.Background() instead.
Replace engine.Unit with component.Component
- Arbitrarily set number of workers to 3
Uses a fifoqueue to buffer tasks (planned to replace with channel,
since we would rather wait to upload than let a block be not uploaded).
Does not correctly propagate errors, or calculate any new metrics yet.
tim-barry and others added 12 commits December 12, 2024 10:02
BadgerRetryableUploadWrapper wraps an AsyncUploader. Now that the
AsyncUploader is a Component, it needs to be `Start()`ed.
The retryable wrapper did not itself do anything special on ready/done,
so the Component functionality is directly delegated to the wrapped AsyncUploader.
Since AsyncUploader now implements Component instead of ReadyDoneAware,
update the methods used to start and end the AsyncUploader (using a context
and its cancel() function).
Test "uploads are run in parallel" currently failing (due to only one worker taking tasks from the queue?)
Instead of using a notifier and fifoqueue with metrics, use a buffered channel.
Block when the channel is full. (Reasoning: we want to ensure no uploads get dropped.)
Buffer size of 100 was chosen arbitrarily. All AsyncUploader tests now pass.
Because Component's `Ready()` and `Done()` methods work differently from
ReadyDoneAware's, include them in Component with Component-specific comments.
Namely, specify that Components should be started with Start() and shutdown
by canceling the context they were started with.
Update/clarify doc comments

Co-authored-by: Jordan Schalm <[email protected]>
…move-Unit

Remove engine.Unit from Assigner engine and Fetcher engine
Channel size of 20000 and worker count of 100 suggested by Leo. 20000 is approximately equal to 4 hours of execution results.

Co-authored-by: Leo Zhang <[email protected]>
…it-refactor

Refactor AsyncUploader to replace Engine.Unit with ComponentManager
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 77.20588% with 31 lines in your changes missing coverage. Please review.

Project coverage is 41.22%. Comparing base (8a3055c) to head (266ad1f).
Report is 245 commits behind head on master.

Files with missing lines Patch % Lines
engine/collection/pusher/engine.go 79.10% 10 Missing and 4 partials ⚠️
...collection/mock/guaranteed_collection_publisher.go 0.00% 8 Missing ⚠️
engine/execution/ingestion/uploader/uploader.go 88.88% 3 Missing and 2 partials ⚠️
engine/verification/fetcher/engine.go 50.00% 2 Missing ⚠️
cmd/collection/main.go 0.00% 1 Missing ⚠️
engine/testutil/mock/nodes.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6833      +/-   ##
==========================================
- Coverage   41.26%   41.22%   -0.04%     
==========================================
  Files        2061     2108      +47     
  Lines      182702   186136    +3434     
==========================================
+ Hits        75384    76737    +1353     
- Misses     101010   102999    +1989     
- Partials     6308     6400      +92     
Flag Coverage Δ
unittests 41.22% <77.20%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tim-barry tim-barry marked this pull request as ready for review December 20, 2024 22:17
@tim-barry tim-barry enabled auto-merge December 20, 2024 22:22
@tim-barry tim-barry added this pull request to the merge queue Dec 20, 2024
Merged via the queue into master with commit 66e6db0 Dec 21, 2024
110 checks passed
@tim-barry tim-barry deleted the feature/pusher-engine-refactor branch December 21, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants