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

custom ProviderManager that brokers AddrInfos #751

Merged
merged 8 commits into from
Oct 26, 2021
Merged

custom ProviderManager that brokers AddrInfos #751

merged 8 commits into from
Oct 26, 2021

Conversation

petar
Copy link
Contributor

@petar petar commented Oct 18, 2021

  • The DHT option ProvidersOptions has been removed. It was used to pass options to the default ProviderManager. Going forward, users can create their own instance of ProviderManager and pass it in using the new ProviderStore DHT option.

Addresses #749 (comment)

@petar petar marked this pull request as ready for review October 18, 2021 20:35
@BigLep BigLep linked an issue Oct 19, 2021 that may be closed by this pull request
providers/providers_manager.go Outdated Show resolved Hide resolved
providers/providers_manager.go Show resolved Hide resolved
providers/providers_manager_test.go Outdated Show resolved Hide resolved
routing.go Outdated Show resolved Hide resolved
@@ -269,7 +284,7 @@ func (pm *ProviderManager) GetProviders(ctx context.Context, k []byte) []peer.ID
case <-ctx.Done():
return nil
case peers := <-gp.resp:
return peers
return peerstore_legacy.PeerInfos(pm.pstore, peers)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use peerstore.AddrInfos here, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The newer peerstore does not have this function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only present in go-libp2p-core v0.9.0 and up, however bumping to this release introduces a ton of other problems. Many of the libraries that this repo depends on, as well as the tests in the repo, are incompatible with v0.9.0 and up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm... ok I guess. We're going to be using the later go-libp2p-core anyway in places like go-ipfs, but we can leave this for now.

In that case I wouldn't call the other one peerstore_legacy, it's where the common peerstore implementations live and definitely isn't legacy. Perhaps peerstore_impl or something like that.

@petar
Copy link
Contributor Author

petar commented Oct 20, 2021

@aschmahmann You can do another review round here.

@aschmahmann
Copy link
Contributor

@petar CI is failing due to TestFindProviderAsync. I suspect you didn't introduce this bug, but can you double check?

Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, aside from one nit + one double check on a test. Merge when ready

providers/providers_manager.go Outdated Show resolved Hide resolved
@petar petar merged commit 7724838 into master Oct 26, 2021
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
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.

Extensible provider record storage/retrieval
2 participants