Support Snappy compression for gRPC #2940

Wing924 · 2020-07-28T05:30:23Z

What this PR does:

Add snappy compression for gRPC.

Which issue(s) this PR fixes:
Fixes #2898

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

fix: cortexproject#2898 Signed-off-by: Wing924 <[email protected]>

Signed-off-by: Wing924 <[email protected]>

pstibrany

Thank you for your work! It looks good. I've left some comments here and there, but nothing major.

pkg/util/grpc/encoding/snappy/snappy.go

pkg/util/grpcclient/grpcclient.go

pstibrany · 2020-07-28T10:06:46Z

pkg/util/grpcclient/grpcclient.go

+func (cfg *Config) Validate(log log.Logger) error {
+	if cfg.UseGzipCompression {
+		flagext.DeprecatedFlagsUsed.Inc()
+		level.Warn(log).Log("msg", "flag *.grpc-use-gzip-compression (or config use_gzip_compression) is deprecated, use *.grpc-use-compression instead.")


Not sure if it's a good idea, but we could save the prefix in RegisterFlagsWithPrefix, and then report full flag name here.

pkg/util/grpcclient/grpcclient.go

CHANGELOG.md

bboreham · 2020-07-28T17:26:15Z

Could you give specific numbers for the benchmarks - looks like a 40% reduction in distributor CPU usage traded for a 150% change in network bandwidth? (comparing gzip and snappy).

Signed-off-by: Wing924 <[email protected]>

Wing924 · 2020-07-29T01:48:56Z

benchmark result

resource	none	gzip	snappy	none -> gzip	none -> snappy	gzip -> snappy
distributor cpu	23%	40%	25%	+74%	+8%	-38%
ingester cpu	43%	46%	49%	+6%	+13%	+7%
distributor network	74Mbps	8.2Mbps	12Mbps	-88%	-83%	+46%
ingester network	93Mbps	6.5Mbps	12Mbps	-93%	-87%	+84%

Wing924 · 2020-07-29T01:49:37Z

@pstibrany I've fixed it. Could you review it again?

pstibrany

Thank you very much for addressing my feedback. I have left some more small comments. Can you please take a look?

pstibrany · 2020-07-29T12:23:14Z

CHANGELOG.md

+* [ENHANCEMENT/CHANGE] Added new flags `-*.grpc-compression` to configure compression used by gRPC. Valid values are "gzip", "snappy", "" (no compression, default). Previous flags (`-*.use-gzip-compression`, enumerate) are now deprecated. #2940
+  * `-bigtable.grpc-compression`
+  * `-ingester.client.grpc-compression`
+  * `-querier.frontend-client.grpc-compression`
+  * Deprecated: `-bigtable.grpc-use-gzip-compression`
+  * Deprecated: `-ingester.client.grpc-use-gzip-compression`
+  * Deprecated: `-querier.frontend-client.grpc-use-gzip-compression`


Suggested change

* [ENHANCEMENT/CHANGE] Added new flags `-*.grpc-compression` to configure compression used by gRPC. Valid values are "gzip", "snappy", "" (no compression, default). Previous flags (`-*.use-gzip-compression`, enumerate) are now deprecated. #2940

* `-bigtable.grpc-compression`

* `-ingester.client.grpc-compression`

* `-querier.frontend-client.grpc-compression`

* Deprecated: `-bigtable.grpc-use-gzip-compression`

* Deprecated: `-ingester.client.grpc-use-gzip-compression`

* Deprecated: `-querier.frontend-client.grpc-use-gzip-compression`

* [ENHANCEMENT] Added new flags `-bigtable.grpc-compression`, `-ingester.client.grpc-compression`, `-querier.frontend-client.grpc-compression` to configure compression used by gRPC. Valid values are `gzip`, `snappy`, or empty string (no compression, default). #2940

* [CHANGE] Flags `-bigtable.grpc-use-gzip-compression`, `-ingester.client.grpc-use-gzip-compression`, `-querier.frontend-client.grpc-use-gzip-compression` are now deprecated. #2940

(Note that they should be moved to their respective sections)

pstibrany · 2020-07-29T12:29:39Z

pkg/util/grpc/encoding/snappy/snappy.go

+	*compressor
+	*snappy.Writer


I'd prefer to avoid nameless embeds. They expose more methods than intended (writeCloser now has methods like Name and Compress), and may inadvertently implement methods that were not meant to be implemented.

Same comment applies to reader.

pkg/util/grpc/encoding/snappy/snappy.go

pstibrany · 2020-07-29T12:34:14Z

pkg/util/grpcclient/grpcclient.go

+func (cfg *Config) Validate(log log.Logger) error {
+	if cfg.UseGzipCompression {
+		flagext.DeprecatedFlagsUsed.Inc()
+		level.Warn(log).Log("msg", fmt.Sprintf("running with DEPRECATED flag -%s.grpc-use-gzip-compression, use -%s.grpc-use-compression instead.", cfg.prefix, cfg.prefix))


should be ... use -%s.grpc-compression now.

pkg/ingester/client/client.go

pkg/util/grpcclient/grpcclient.go

pkg/util/grpc/encoding/snappy/snappy.go

pracucci

Amazing job, thanks! I left few comments (other than Peter's ones) but I believe we're on a very good track 👏

pracucci · 2020-07-29T15:26:32Z

pkg/util/grpcclient/grpcclient.go

 	MaxRecvMsgSize     int     `yaml:"max_recv_msg_size"`
 	MaxSendMsgSize     int     `yaml:"max_send_msg_size"`
 	UseGzipCompression bool    `yaml:"use_gzip_compression"`
+	Compression        string  `yaml:"compression"`


CLI flags and YAML config option names should match.

Suggested change

Compression string `yaml:"compression"`

Compression string `yaml:"grpc_compression"`

pkg/util/grpcclient/grpcclient.go

pracucci · 2020-07-29T15:51:56Z

pkg/util/grpcclient/grpcclient.go

+		level.Warn(log).Log("msg", fmt.Sprintf("running with DEPRECATED flag -%s.grpc-use-gzip-compression, use -%s.grpc-use-compression instead.", cfg.prefix, cfg.prefix))
+	}
+	switch cfg.Compression {
+	case "gzip", "snappy", "":


I would use snappy.Name instead of the hardcoded "snappy" given we have the constant.

So this made me go back and look why I didn't use gzip.Name, and it turns out it didn't exist at the time.

pracucci · 2020-07-29T15:52:23Z

pkg/util/grpc/encoding/snappy/snappy.go

+	*compressor
+	*snappy.Writer


Same comment applies to reader.

pracucci · 2020-07-29T15:56:53Z

pkg/chunk/storage/factory.go

 		return errors.Wrap(err, "invalid Cassandra Storage config")
 	}
+	if err := cfg.GCPStorageConfig.Validate(util.Logger); err != nil {
+		return errors.Wrap(err, "invalid GCP Storage Storage config")


Suggested change

return errors.Wrap(err, "invalid GCP Storage Storage config")

return errors.Wrap(err, "invalid GCP Storage config")

Signed-off-by: Wing924 <[email protected]>

pstibrany

Thanks for addressing my feedback.

pkg/util/grpc/encoding/snappy/snappy.go

pstibrany · 2020-07-30T07:10:41Z

pkg/util/grpc/encoding/snappy/snappy.go

+func (r *reader) Read(p []byte) (n int, err error) {
+	n, err = r.Reader.Read(p)
+	if err == io.EOF {
+		r.pool.Put(r)


Nit: should we reset snappy.Reader with nil reader before putting back to pool? (same for writer) It would be little more correct to avoid keeping reference to just-read underlying reader, but likely not a big problem as they will be reused all the time.

pstibrany · 2020-07-30T07:14:13Z

pkg/util/grpc/encoding/snappy/snappy.go

+}
+
+type reader struct {
+	*snappy.Reader


Nit: These anonymous embeds now expose snappy.Reader methods. It doesn't even bring anything, as Read is fully reimplemented by reader. In writeCloser it saves reimplementing Write method, although I would still prefer to be explicit. Not a blocker for merging.

pracucci

Nice job! I have last few comments (+ Peter's nits) and then we're good to go 🚀

pracucci · 2020-07-30T09:33:36Z

docs/configuration/config-file-reference.md

+    # Use compression when sending messages. Supported values are: 'gzip',
+    # 'snappy' and '' (disable compression)
+    # CLI flag: -bigtable.grpc-compression
+    [compression: <string> | default = ""]


I would expect this to be grpc_compression. I'm not sure why the CI linter passed. Could you run make doc again, please?

as you may know, CI don't run.

Oh, I haven't noticed! Can't understand why the CI is not running on this specific PR 👀

pracucci · 2020-07-30T09:34:25Z

pkg/util/grpcclient/grpcclient.go

+	compression := cfg.GRPCCompression
 	if cfg.UseGzipCompression {
-		opts = append(opts, grpc.UseCompressor("gzip"))
+		compression = "gzip"


Suggested change

compression = "gzip"

compression = gzip.Name

pracucci · 2020-07-30T09:35:31Z

pkg/util/grpcclient/grpcclient.go

 type Config struct {
 	MaxRecvMsgSize     int     `yaml:"max_recv_msg_size"`
 	MaxSendMsgSize     int     `yaml:"max_send_msg_size"`
 	UseGzipCompression bool    `yaml:"use_gzip_compression"`


Suggested change

UseGzipCompression bool `yaml:"use_gzip_compression"`

UseGzipCompression bool `yaml:"use_gzip_compression"` // TODO: Remove this deprecated option in v1.6.0.

Signed-off-by: Wing924 <[email protected]>

pracucci

I don't know why, but CI is not running on this PR. I'm falling it as "request changes" because we need to get the CI pass before merging.

Signed-off-by: Wing924 <[email protected]>

pracucci · 2020-07-31T08:45:20Z

I don't know why, but CI is not running on this PR. I'm falling it as "request changes" because we need to get the CI pass before merging.

I've found the issue. Your PR description contained some characters スクリーンショット which apparently cause the CircleCI webhook to return 400. I've removed them and force pushed to trigger the CI.

FYI, I've rebased the last commit to reword the commit message by 1 character and git push --force it in order to trigger the CI again.

pracucci

LGTM! One final nit and we should be good to go 😎

pkg/util/grpc/encoding/snappy/snappy.go

…support_snappy_grpc

Signed-off-by: Wing924 <[email protected]>

pstibrany · 2020-07-31T14:40:57Z

Thank you!

Support Snappy compression for gRPC

270a06d

fix: cortexproject#2898 Signed-off-by: Wing924 <[email protected]>

pull-request-size bot added the size/L label Jul 28, 2020

changelog

5df98c9

Signed-off-by: Wing924 <[email protected]>

pstibrany reviewed Jul 28, 2020

View reviewed changes

Wing924 added 2 commits July 29, 2020 08:38

Merge branch 'master' into support_snappy_grpc

835a6d1

fix

5dd90d3

Signed-off-by: Wing924 <[email protected]>

pstibrany reviewed Jul 29, 2020

View reviewed changes

pracucci reviewed Jul 29, 2020

View reviewed changes

Wing924 added 5 commits July 30, 2020 09:45

Merge branch 'master' into support_snappy_grpc

c775a23

fix

6cae329

Signed-off-by: Wing924 <[email protected]>

fix

87bcb12

Signed-off-by: Wing924 <[email protected]>

fix

0f7a209

Signed-off-by: Wing924 <[email protected]>

fix

97bd88d

Signed-off-by: Wing924 <[email protected]>

pstibrany approved these changes Jul 30, 2020

View reviewed changes

pstibrany reviewed Jul 30, 2020

View reviewed changes

pracucci reviewed Jul 30, 2020

View reviewed changes

Wing924 added 2 commits July 31, 2020 16:33

Merge branch 'master' into support_snappy_grpc

0978fec

fix

2f15674

Signed-off-by: Wing924 <[email protected]>

pracucci suggested changes Jul 31, 2020

View reviewed changes

Fix

47d77a0

Signed-off-by: Wing924 <[email protected]>

pracucci force-pushed the support_snappy_grpc branch from 2f15674 to 47d77a0 Compare July 31, 2020 08:44

pracucci approved these changes Jul 31, 2020

View reviewed changes

pkg/util/grpc/encoding/snappy/snappy.go Outdated Show resolved Hide resolved

Wing924 added 3 commits July 31, 2020 18:59

Merge branch 'support_snappy_grpc' of github.com:Wing924/cortex into …

6b4b2bd

…support_snappy_grpc

Merge branch 'master' into support_snappy_grpc

fee4259

fix

88d34a5

Signed-off-by: Wing924 <[email protected]>

pstibrany merged commit 48e74f6 into cortexproject:master Jul 31, 2020

Wing924 deleted the support_snappy_grpc branch July 31, 2020 14:56

pracucci mentioned this pull request Aug 27, 2020

Do not mention deprecated config option use_gzip_compression in the doc #3099

Merged

3 tasks

fpetkovski mentioned this pull request Aug 5, 2022

Receiver: reduce network bandwidth when ingesting data thanos-io/thanos#5572

Closed

	Compression string `yaml:"compression"`
	Compression string `yaml:"grpc_compression"`

	return errors.Wrap(err, "invalid GCP Storage Storage config")
	return errors.Wrap(err, "invalid GCP Storage config")

	UseGzipCompression bool `yaml:"use_gzip_compression"`
	UseGzipCompression bool `yaml:"use_gzip_compression"` // TODO: Remove this deprecated option in v1.6.0.

Support Snappy compression for gRPC #2940

Support Snappy compression for gRPC #2940

Uh oh!

Conversation

Wing924 commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bboreham commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wing924 commented Jul 29, 2020

benchmark result

Uh oh!

Wing924 commented Jul 29, 2020

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

pracucci commented Jul 31, 2020

Wing924 commented Jul 28, 2020 •

edited

Loading

bboreham commented Jul 28, 2020 •

edited

Loading