-
Notifications
You must be signed in to change notification settings - Fork 186
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
make wire protocol message size configurable. #261
Conversation
cc @vishalchangraniaxiom |
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, although I'd like to give @vyzo an opportunity to verify he's ok with this change. This seems to be a solution to a request made at libp2p/specs#118 (comment)
@@ -2,7 +2,7 @@ module github.com/libp2p/go-libp2p-pubsub | |||
|
|||
require ( | |||
github.com/gogo/protobuf v1.3.1 | |||
github.com/hashicorp/golang-lru v0.5.4 | |||
github.com/hashicorp/golang-lru v0.5.4 // indirect |
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.
Is this an artifact from your local branch or do we need to change the dependency to indirect anyway?
psubs := getPubsubs(ctx, hosts, WithMaxMessageSize(1<<22)) | ||
|
||
sparseConnect(t, hosts) | ||
time.Sleep(time.Millisecond * 100) |
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.
These make me sad and they're all over this code base. While we could certainly improve the API (e.g. by allowing a Readiness check even without a dummy discovery mechanism) we can already remove the sleep by following the pattern in this test.
go-libp2p-pubsub/topic_test.go
Line 122 in a1999db
sender := getPubsub(ctx, hosts[0], WithDiscovery(&dummyDiscovery{})) |
go-libp2p-pubsub/topic_test.go
Line 145 in a1999db
if err := sendTopic.Publish(ctx, firstMsg, WithReadiness(MinTopicSize(1))); err != nil { |
Enough approvals; merging. |
Relates to #209, but doesn't close it because this implements a configurable global maximum message size.
This PR introduces the
WithMaxMessageSize(size int)
functi onal option that can be used in the pubsub constructor. It also introduces a unit test, and detailed godocs for this feature.