-
Notifications
You must be signed in to change notification settings - Fork 12
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
Remove bootstrapping code #37
Conversation
3561865
to
2fcdcce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you write a test to stress concurrent PutValue
calls?
2fcdcce
to
45be97b
Compare
26324c6
to
e81940b
Compare
c9170fb
to
e6a6cc9
Compare
Before I merge this it'd be nice to be able to run CI on the go-ipfs PR that incorporates the change, for extra testing. However, that's currently blocked bc this repo was updated to the latest version of the datastore interface. @Stebalien should I just merge this and we can worry about the integration later? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit - otherwise LGTM
…ce we can just use pubsub discovery
e6a6cc9
to
4c1aadd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Now that PubSub has an optional discovery service built into it seems reasonable for the PubsubValueStore to assume that the PubSub instance passed into it utilizes this discovery mechanism (if that is in fact what the caller intends). This closes #28.
While we're doing breaking the constructor and semantics anyway (removing the mandatory ContentRouter and needing the PubSub instance passed in to have Discovery enabled) I also added an option pattern to the constructor.