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

update the peerstore, change the peerstore option #1256

Closed
wants to merge 1 commit into from

Conversation

marten-seemann
Copy link
Contributor

No description provided.

@@ -148,6 +148,9 @@ type HostOpts struct {
// DisableSignedPeerRecord disables the generation of Signed Peer Records on this host.
DisableSignedPeerRecord bool

// EventBus must be set.
EventBus event.Bus
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is annoying. NewHost will panic if the EventBus is not set.
Should we make the event.Bus a function parameter of NewHost?

Copy link
Contributor

Choose a reason for hiding this comment

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

that sounds like the right thing to do

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

I am beginning to suspect we should have put the pstore manager in the host ans avoided this mess... oh well.

@marten-seemann
Copy link
Contributor Author

I am beginning to suspect we should have put the pstore manager in the host ans avoided this mess... oh well.

I had the same thought. But logically, it does belong to the peerstore and not to the host. I suppose the long-term answer here is dependency injection, but I don't want to cross that bridge now.

@burdiyan
Copy link
Contributor

burdiyan commented Dec 8, 2021

Agree very much with @vyzo on: #1256 (review).

FWIW, quoting myself from libp2p/go-libp2p-peerstore#186 (comment):

I'd argue the opposite, actually.

IMHO, logically the cleanup work does belong more to the Host than to the Peerstore. Peerstore just deals with storage and should not be responsible for cleaning things up. It does provide the means to remove records but it could be free from the responsibility of doing that. Actually this is how it is already, since the logic is actually implemented in a separate PeerstoreManager type, and I do think that it's a good design decision.

But in terms of who's responsible for creating and starting the PeerstoreManager I strongly believe that architecturally it's a lot cleaner if Host is responsible for that, or even the caller who creates the Host. There're already lots of things that are encapsulated inside the Host interface, and this could be one more. It also helps with understanding of things, when you know that all the side-effects and background-related routines are initiated in one place. It also makes implementing Peerstore interface easier, because you don't have to think about creating and starting the PeerstoreManager in every Peerstore implementation, which is a thing that could be easy to forget.

Of course all of this is subjective, and both approaches will do the job. I just wanted to bring up some thoughts and ideas as a person who's not involved with Protocol Labs, had been working with Libp2p and IPFS for couple years now, using the most lower-level components available and assembling them manually. And I honestly find lots of confusing things in terms of responsibilities between all the different components.

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.

3 participants