refactor: make disperser client reuse same grpc connection #826
Conversation
| } | ||
|
|
||
| var _ DisperserClient = &disperserClient{} | ||
|
|
||
| func NewDisperserClient(config *Config, signer core.BlobRequestSigner) DisperserClient { | ||
| // DisperserClient maintains a single underlying grpc connection to the disperser server, | ||
| // through which it sends requests to disperse blobs, get blob status, and retrieve blobs. |
There was a problem hiding this comment.
probably should take care of the max concurrent requests, see https://learn.microsoft.com/en-us/aspnet/core/grpc/performance?view=aspnetcore-8.0#connection-concurrency.
If it reaches the max, the new call get blocked. It can happen if OP uses concurrent blobs spinning up 100 conn
There was a problem hiding this comment.
Ah good catch. We can bump that limit or make it a configuration actually.
But I really don't think it's that big of a worry. OP would have to literally make 100 requests at the same time, which I think is unlikely? How long does a typical DisperseBlobAuthenticated take? < 5secs hopefully?
Also what happens if that limit is reached? it returns a 500 or something? Then batcher would just resubmit the blob a little bit later once the other connections are done processing.
There was a problem hiding this comment.
DisperseBlobAuthenticated takes about 300ms for 1MB blob
There was a problem hiding this comment.
Update comment: ef1c7a5
| // | ||
| // // Subsequent calls will use the existing connection | ||
| // status2, requestId2, err := client.DisperseBlob(ctx, otherData, otherQuorums) | ||
| func NewDisperserClient(config *Config, signer core.BlobRequestSigner) *disperserClient { |
There was a problem hiding this comment.
Why did we switch the return type to struct pointer vs. interface?
There was a problem hiding this comment.
https://en.m.wikipedia.org/wiki/Robustness_principle instantiated in golang is "take interfaces as input, and return structs" (see https://bryanftan.medium.com/accept-interfaces-return-structs-in-go-d4cab29a301b for eg).
This is not a breaking change because the pointer implements the interface.
There was a problem hiding this comment.
This is a good read. Thanks!
No we don't! :O What do you recommend is best way to test the disperser client? Writing a fake grpc server is prob the easiest way to go right? EDIT: spoke with @ian-shim, we just merged #820. I just rebased so e2e test should run on this branch to at least test disperser_client. |
samlaf
force-pushed
the
samlaf/refactor--make-disperser-client-reuse-same-grpc-connection
branch
from
79e97f1
to
fb3d28d
Compare
samlaf
deleted the
samlaf/refactor--make-disperser-client-reuse-same-grpc-connection
branch
Why are these changes needed?
grpc "connections" are actually virtual connections: the docs/specs calls them channels.
They are not a physical underlying tcp/http connection but actually manage underlying connections. Hence, they are not meant to be redialed on every request. They should be created once, and reused for multiple requests, which is what this PR does.
Checks