-
Notifications
You must be signed in to change notification settings - Fork 131
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
drainer,pump: Add support for enabling gzip grpc compression #495
Conversation
/run-all-tests tidb=release-2.1 tikv=release-2.1 pd=release-2.1 |
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.
rest LGTM
@@ -83,6 +85,8 @@ func NewServer(cfg *Config) (*Server, error) { | |||
} | |||
|
|||
ctx, cancel := context.WithCancel(context.Background()) | |||
ctx = context.WithValue(ctx, drainerKeyType("compressor"), cfg.Compressor) |
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.
would it be better just pass by add param fro NewPump?
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.
I've tried to get this piece of information from cfg
in Server
and pass it all the way down to Pump
.
But it turned out I would also have to add a corresponding field in Collector
and Pump
because the part that may need to know compressor
was not called when these structs were created.
So I guess using context.WithValue
might be a little simpler, for example, when I changed enable_compression
to compressor
I only need to update two or three lines of code.
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
@WangXiangUSTC @GregoryIan PTAL |
rest LGTM |
/run-all-tests tidb=release-2.1 tikv=release-2.1 pd=release-2.1 |
@WangXiangUSTC PTAL |
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
What problem does this PR solve?
Add support for enabling gzip compression of payloads between Pump and Drainer Issue
What is changed and how it works?
A new flag
compressor
is added, which can be"gzip"
or""
(compressor disabled).The value is
""
by default, so original users won't get compression turned on by accident after upgrading to a version that include this change.Check List
Tests
stats.Handler
is used to verify that the payload is indeed compressedCode changes
Side effects
Related changes