encoding: add compressor options in UseCompressor and SetSendCompressor#9036
encoding: add compressor options in UseCompressor and SetSendCompressor#9036Dostonlv wants to merge 1 commit intogrpc:masterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9036 +/- ##
==========================================
- Coverage 83.01% 82.25% -0.77%
==========================================
Files 413 413
Lines 33054 33315 +261
==========================================
- Hits 27441 27403 -38
- Misses 4205 4254 +49
- Partials 1408 1658 +250
🚀 New features to boost your workflow:
|
|
@easwars assalamu alaykum could you review my code? |
Pranjali-2501
left a comment
There was a problem hiding this comment.
Hi @Dostonlv , Thanks for raising the PR.
I have added a few comments, PTAL.
36a0335 to
a572da0
Compare
a572da0 to
6d65379
Compare
|
@Pranjali-2501 Assalamu alaykum added tests to verify compressor options are correctly propagated. Please review. |
|
@arjan-bal, assigning it to you for second review. |
|
I left a few questions on the proposal: #7017 (comment) I'll hold off on the PR review until we get confirmation on those points. |
| type wrapCompressor struct { | ||
| encoding.Compressor | ||
| compressInvokes int32 | ||
| mu sync.Mutex |
There was a problem hiding this comment.
Let's use the mutex to protect compressInvokes too. Unifying the synchronization mechanisms will make the code much simpler.
Additionally, please use empty lines to group the mutex-guarded fields, similar to this example:
grpc-go/internal/balancer/gracefulswitch/gracefulswitch.go
Lines 50 to 63 in 89fe783
| {"unary", testUnarySendCompressorOptionsPropagate}, | ||
| {"stream", testStreamSendCompressorOptionsPropagate}, |
There was a problem hiding this comment.
style: The helper seems to be abstracting most of the test logic here. I would recommend avoiding the table driven approach and have separate tests for unary and streaming RPCs, inlining the helpers. Same for the TestUseCompressorOptionsPropagate test.
See https://google.github.io/styleguide/go/best-practices#leave-testing-to-the-test-function
| if len(wc.receivedOpts) == 0 { | ||
| t.Fatal("Compress was not called") | ||
| } | ||
| if got := wc.receivedOpts[0]; len(got) == 0 || got[0] != wantOpt { |
There was a problem hiding this comment.
nit: You can use cmp.Diff to compare the entire slice.
|
After discussing this with the other maintainers, we agreed that we need to provide a migration path for custom compressor implementers rather than breaking the API in a single release. We will need a transitional gRPC version where both the old and new APIs coexist before we fully remove the old one. I am currently evaluating the best approach to achieve this. |
Fixes #7017
RELEASE NOTES: