Skip to content
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

Adding GRPC S2 compression implementation #582

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

deniszh
Copy link

@deniszh deniszh commented Sep 5, 2024

What this PR does:

Adding S2 compression into GRPC encoding.

Which issue(s) this PR fixes:

Fixes #8522

Checklist

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

Implementation mostly taken from https://github.com/mostynb/go-grpc-compression/blob/main/internal/s2/s2.go
Also added snappy grpc compression benchmark, to match with S2:

goos: darwin
goarch: arm64
pkg: github.com/grafana/dskit/grpcencoding/s2
cpu: Apple M1
BenchmarkS2SCompress-8             	  249318	      4038 ns/op
BenchmarkS2Decompress-8            	 9593479	       120.8 ns/op
BenchmarkS2GrpcCompressionPerf-8   	    9314	    120118 ns/op
PASS
ok  	github.com/grafana/dskit/grpcencoding/s2	4.409s
goos: darwin
goarch: arm64
pkg: github.com/grafana/dskit/grpcencoding/snappy
cpu: Apple M1
BenchmarkSnappyCompress-8              	  373032	      2785 ns/op
BenchmarkSnappyDecompress-8            	 5234221	       250.9 ns/op
BenchmarkSnappyGrpcCompressionPerf-8   	   12583	     93050 ns/op
PASS
ok  	github.com/grafana/dskit/grpcencoding/snappy	7.214s

@CLAassistant
Copy link

CLAassistant commented Sep 5, 2024

CLA assistant check
All committers have signed the CLA.

@aknuds1
Copy link
Collaborator

aknuds1 commented Sep 11, 2024

Looks as if you need to run go mod tidy.

@aknuds1 aknuds1 added the enhancement New feature or request label Sep 11, 2024
@aknuds1
Copy link
Collaborator

aknuds1 commented Sep 12, 2024

I got some more feedback from @pstibrany, we might want to support S2 directly in Mimir instead of going via dskit (to avoid bloating dskit's dependencies). Could you open a (draft?) PR to Mimir, so we can try to come up with a solution there? Thanks!

@deniszh
Copy link
Author

deniszh commented Sep 12, 2024

Sure, let me do that

@deniszh
Copy link
Author

deniszh commented Sep 12, 2024

@aknuds1 : not sure how to do that on Mimir level, will try to do that tomorrow.
But btw, which dependencies increase are you talking about? I didn't add new modules, s2 already included in klauspost compress repo. I see only minor upgrade, which IMO should be done anyway.

@aknuds1
Copy link
Collaborator

aknuds1 commented Sep 13, 2024

not sure how to do that on Mimir level, will try to do that tomorrow.

What @pstibrany suggests is to add a dskit extension point, to allow configuring a custom compression type from Mimir.

If you just open a draft Mimir PR depending on this dskit branch, I'll help you shape it into something acceptable :) It's easier for me to see how to do it when I have a Mimir PR.

@deniszh
Copy link
Author

deniszh commented Sep 13, 2024

Looks as if you need to run go mod tidy.

Ah, for example app. Just did, together with rebase.

@deniszh
Copy link
Author

deniszh commented Sep 13, 2024

If you just open a draft Mimir PR depending on this dskit branch, I'll help you shape it into something acceptable :) It's easier for me to see how to do it when I have a Mimir PR.

Sure, I can do that. I didn't do that before because latest Mimir was not compatible with latest dskit, let me try one more time.

I created 2 special branches in dskit for 2.12.0 ans 2.13.0 releases, based on corresponding dskit commits:

@deniszh
Copy link
Author

deniszh commented Sep 13, 2024

Opened draft PR - grafana/mimir#9292

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zstd compression support between distributors and ingesters
3 participants