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

Deprecate 'querier.ingester-metadata-streaming' flag #6147

Conversation

alanprot
Copy link
Member

@alanprot alanprot commented Aug 9, 2024

What this PR does:
Deprecate and enable by default querier.ingester-metadata-streaming flag and mark to be removed after v1.18.

This flag is around and stable for more than 2 years and there is no reason to be disabled.

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@alanprot alanprot force-pushed the deprecate/querier.ingester-metadata-streaming branch from 259831a to 020050e Compare August 9, 2024 18:20
@alanprot alanprot marked this pull request as ready for review August 9, 2024 18:20
@alanprot alanprot requested a review from yeya24 August 9, 2024 20:34
@@ -108,7 +108,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
cfg.StoreGatewayClient.RegisterFlagsWithPrefix("querier.store-gateway-client", f)
f.IntVar(&cfg.MaxConcurrent, "querier.max-concurrent", 20, "The maximum number of concurrent queries.")
f.DurationVar(&cfg.Timeout, "querier.timeout", 2*time.Minute, "The timeout for a query.")
f.BoolVar(&cfg.IngesterMetadataStreaming, "querier.ingester-metadata-streaming", false, "Use streaming RPCs for metadata APIs from ingester.")
f.BoolVar(&cfg.IngesterMetadataStreaming, "querier.ingester-metadata-streaming", true, "Deprecated (This feature will be always on after v1.18): Use streaming RPCs for metadata APIs from ingester.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use flagext.DeprecatedFlag instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanna keep the flag functional for 1 release.
DeprecatedFlag is used when we removed the flag entirely.

Do u wanna remove the flag at once -> i did not do as, until now, the default was false. =/

Copy link
Contributor

Choose a reason for hiding this comment

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

I see make sense.

@alanprot alanprot merged commit c49d61f into cortexproject:master Aug 9, 2024
16 checks passed
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.

2 participants