fix: run publisher in its own task #188
Conversation
WS1-1339 DHT flaky tests
We believe the DHT publish logic is starving the swarm task. We should run it on its own task. DOD:
|
|
Locally this works well, however I'd like to test in dev/QA with better load to determine how well it works. We can revert if its not sufficient. |
| let mut stream = PublisherStream::new(results_rx, interval, block_store, metrics.clone()); | ||
| let task_metrics = metrics.clone(); | ||
| tokio::spawn(async move { | ||
| while let Some(batch) = stream.next().await { |
There was a problem hiding this comment.
So:
- The
PublisherdrivesPublisherStreamin a separate task. PublisherStreambuilds and returns batches into a channel that gets polled by the swarm task.- The swarm publishes a batch and returns results via another channel that gets consumed first before a new batch is returned.
Does this sound like an accurate description?
There was a problem hiding this comment.
And it's the SQL lookups/batching logic that's been moved out from the swarm task to its own task, via PublisherStream.
There was a problem hiding this comment.
PublisherStream builds and returns batches into a channel that gets polled by the swarm task.
This part is technically incorrect. Both the Publisher and PublisherStream types implement the Stream trait. The swarm is unware that PublisherStream exists or that the channel exists. So the swarm task polls the Publisher for the next batch. The only work the Publisher does is to read a channel if something is available.
The swarm publishes a batch and returns results via another channel that gets consumed first before a new batch is returned.
Yes except the swarm is unware of the results channel as well. It simply calls the handle method on the Publisher which puts the result on a channel that the PublisherStream reads.
So generally correct except that the Publisher abstraction means that the swarm doesn't care that channels and a separate task are being used under the hood.
Perhaps I should rename PublisherStream to PublisherWorker? That way its not misleading that they both implement Stream?
There was a problem hiding this comment.
Ah gotcha, makes sense, ty! I got confused by the two layers of streams.
Yeah, I think calling it PublisherWorker would help.
There was a problem hiding this comment.
Or perhaps even PublisherStream and PublisherWorkerStream but not a strong preference.
There was a problem hiding this comment.
I changed the names to Publisher and PublisherWorker. Keep it simple. I have added comments about the fact they implement the Stream trait and why.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This change makes the Publisher logic run in its own task. This is to relieve CPU pressure from the swarm task.
nathanielc
force-pushed
the
feature/ws1-1339-dht-flaky-tests
branch
from
12f93cb
to
92fce7b
Compare
This change makes the Publisher logic run in its own task. This is to relieve CPU pressure from the swarm task.