feat: set provider manager options #593
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
willscott
reviewed
Apr 20, 2020
2 tasks
willscott
reviewed
Apr 21, 2020
| @@ -56,22 +86,23 @@ type getProv struct { | |||
| } | |||
|
|
|||
| // NewProviderManager constructor | |||
| func NewProviderManager(ctx context.Context, local peer.ID, dstore ds.Batching) *ProviderManager { | |||
| func NewProviderManager(ctx context.Context, local peer.ID, dstore ds.Batching, opts ...Option) (*ProviderManager, error) { | |||
| pm := new(ProviderManager) | |||
| pm.getprovs = make(chan *getProv) | |||
| pm.newprovs = make(chan *addProv) | |||
| pm.dstore = autobatch.NewAutoBatching(dstore, batchBufferSize) | |||
| cache, err := lru.NewLRU(lruCacheSize, nil) | |||
There was a problem hiding this comment.
hydra is probably the only case where there's a worry about this being expensive, so if this optimization doesn't seem important there, it may not be worth the complexity.
consider checking if the cache is unset after applying options, and only creating the fallback/default one if it hasn't.
|
Any reason not to use a single shared provider record instead of constructing one per DHT? |
1 similar comment
|
Any reason not to use a single shared provider record instead of constructing one per DHT? |
|
Do you mean a single provider manager? I did originally try that, please read the PR description for why not. |
|
I did not read it... you're right, this seems like the right way to go. |
Stebalien
approved these changes
Apr 21, 2020
alanshaw
pushed a commit
to libp2p/hydra-booster
that referenced
this pull request
Apr 22, 2020
This adds a new flag and env var to allow provider record GC to be disabled: `-disable-prov-gc` / `HYDRA_DISABLE_PROV_GC`. If GC is _enabled_ it automatically disables provider record GC for all Hydra heads other than the first. It also renames the `Relay` option to `EnableRelay` to better communicate that it's a boolean flag. Depends on: * [x] libp2p/go-libp2p-kad-dht#593 * [x] sysulq/golang-lru#6 fixes #74
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR enables the provider manager to accept options and exposes the
cleanupIntervalandcache(cache implementation) as options that can be set.This also adds
providersOptions []providers.Optionas a main option to the DHT.Context
Currently all Hydra heads share a datastore and they all perform GC on that same datastore. We really only need one Hydra head to perform GC. Exposing the
cleanupIntervaloption allows us to disable GC on all other heads (by setting it really high).Originally I opened #591 to expose the
ProviderManageras a constructor option, the idea being to construct a single provider manager instance and pass it to all DHTs. However, this would have only solved the GC problem for a single Hydra i.e. we wouldn't be able to stop multiple Hydras sharing the same datastore from GC-ing and additionally, all heads for a single Hydra would share the same cache. Since Hydra Peer IDs are balanced, the overlap between the caches should be minimal and we'd end up with a thrashy cache not tuned for a particular peer ID.The cache implementation is exposed as an option because Hydra heads who are not performing GC still need a way to invalidate their caches (the cache is cleared only when GC begins). An LRU cache with expiry will be passed instead (maybe this one https://github.com/hnlq715/golang-lru).
NewProviderManagernow returns an error instead of panicing...