Conversation
guillaumemichel
added
the
skip/changelog
guillaumemichel
force-pushed
the
fix/provider-merge-conflicts
branch
from
b2ed9a6 to
2b5ec42
Compare
- add Provide.DHT.BufferedBatchSize (default: 1024) - add Provide.DHT.BufferedIdleWriteTime (default: 5s) - improves UX: content becomes discoverable faster (5s vs 60s) - allows operators to tune batching for their workload
core/node/provider.go
Outdated
| bufferedProviderOpts := []buffered.Option{ | ||
| buffered.WithBatchSize(int(cfg.Provide.DHT.BufferedBatchSize.WithDefault(config.DefaultProvideDHTBufferedBatchSize))), | ||
| buffered.WithDsName("bprov"), | ||
| buffered.WithIdleWriteTime(cfg.Provide.DHT.BufferedIdleWriteTime.WithDefault(config.DefaultProvideDHTBufferedIdleWriteTime)), |
There was a problem hiding this comment.
waiting 60s for a flush to trigger provide after ipfs add would make people report bugs that providing is broken.
@guillaumemichel i changed this to 5s and made it configurable – does 72c4693 make sense, or did I misunderstood the impact of this 1 minute flush delay?
There was a problem hiding this comment.
No, this is the IdleWriteTime for the go-dsqueue used by the buffer.
It means that it will hold the multihashes it receives in memory, and if it doesn't receive new items in >1 minute, it will persist items from the queue that are in memory to the datastore.
The provides happen as fast as possible. There is a worker dedicated to taking multihashes from the buffer (in front of the SweepingProvider) and adding them to the provide queue and schedule.
I don't really have a good intuition on what this value should be, so I took the default one (1 minute), that is also used in the boxo/provider queue. Maybe @gammazero has a better suggestion?
There was a problem hiding this comment.
Thanks. I realized 1m is DefaultIdleWriteTime https://github.com/ipfs/go-dsqueue.
I've reverted in 83b280c. and only documented bufferedBatchSize and bufferedIdleWriteTime (back to 1m) as consts.
Better to not expose too much internals to users, as that often backfires when users put random values. If we se the need, we can expose those under Internal.* or something in the future.
This reverts commit 72c4693. Moved BufferedBatchSize and BufferedIdleWriteTime from user configuration to hardcoded constants in core/node/provider.go. These are internal implementation details that users don't need to tune - the upstream defaults (1024 batch size, 1 minute idle time) are battle-tested and provide optimal performance for most use cases.
Co-authored-by: Marcin Rataj <lidel@lidel.org> (cherry picked from commit 776c21a)
2 participants
Fixes #10988
When resolving merge conflicts between #10951 and
master, some parts of the code have been overridden.As a result, the
buffered.SweepingProviderusage has been removed, triggering #10988.This PR reverts the changes as they were initially in #10834. Hopefully, I didn't miss anything critical.