Skip to content

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Sep 21, 2020

Signed-off-by: Ben Ye [email protected]

What this PR does:

Add an enable_snappy config to Results Cache.

Which issue(s) this PR fixes:
Fixes #

Checklist

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would allow to configure the compression algorithm name instead of a bool. We did this make for the gRPC compression in the past, which then lead to deprecating the boolean in favour of a string:

    # Deprecated: Use gzip compression when sending messages.  If true,
    # overrides grpc-compression flag.
    # CLI flag: -bigtable.grpc-use-gzip-compression
    [use_gzip_compression: <boolean> | default = false]

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

I would suggest to go direct with a compression string which we'll accept snappy and empty string (disabled, default).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds much better! Thanks, I will update my pr and let you know.

@yeya24 yeya24 force-pushed the support-results-cache-snappy branch from 134e365 to 9f54f66 Compare September 22, 2020 20:38
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@yeya24 yeya24 force-pushed the support-results-cache-snappy branch from 74cf1d6 to a2c7e61 Compare September 23, 2020 13:42
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@pracucci pracucci merged commit 8587ea6 into cortexproject:master Sep 23, 2020
@yeya24 yeya24 deleted the support-results-cache-snappy branch September 23, 2020 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants