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 -distributor.shard-by-all-labels #6022

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tesla59
Copy link
Contributor

@tesla59 tesla59 commented Jun 17, 2024

What this PR does:
Remove the flag distributor.shard-by-all-labels and sets it to true by default

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

Checklist

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

pkg/distributor/distributor.go Show resolved Hide resolved
docs/configuration/arguments.md Outdated Show resolved Hide resolved
docs/configuration/arguments.md Outdated Show resolved Hide resolved
docs/configuration/arguments.md Outdated Show resolved Hide resolved
docs/configuration/arguments.md Outdated Show resolved Hide resolved
@tesla59 tesla59 force-pushed the tesla/deprecate-sharedbyall-flag branch from 7d1e9b6 to f4693b7 Compare June 17, 2024 12:03
@tesla59
Copy link
Contributor Author

tesla59 commented Jun 17, 2024

Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

Thank you! only nits now

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/distributor/distributor.go Outdated Show resolved Hide resolved
@friedrichg
Copy link
Member

@tesla59 the lint needs to pass too
An error occurred while generating the doc: config=github.com/cortexproject/cortex/pkg/distributor.Config: unable to find CLI flag for 'ShardByAllLabels' config entry exit status 1

Signed-off-by: tesla59 <nishant@heim.id>
@tesla59 tesla59 force-pushed the tesla/deprecate-sharedbyall-flag branch from f4693b7 to b05e549 Compare June 17, 2024 14:38
Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

Sorry, I did another pass and these are still related and should be removed:

grep -R ShardByAllLabels *
pkg/cortex/cortex.go:	if err := c.LimitsConfig.Validate(c.Distributor.ShardByAllLabels); err != nil {
pkg/cortex/modules.go:	t.Cfg.Ingester.DistributorShardByAllLabels = t.Cfg.Distributor.ShardByAllLabels
pkg/ingester/ingester.go:	DistributorShardByAllLabels bool   `yaml:"-"`
pkg/ingester/ingester.go:		cfg.DistributorShardByAllLabels,
pkg/ingester/ingester.go:		cfg.DistributorShardByAllLabels,
pkg/distributor/query.go:	if !d.cfg.ShardByAllLabels && len(matchers) > 0 {
pkg/distributor/distributor.go:	ShardByAllLabels         bool   `yaml:"shard_by_all_labels"`
pkg/distributor/distributor.go:	cfg.ShardByAllLabels = true
pkg/distributor/distributor.go:	if d.cfg.ShardByAllLabels {
pkg/distributor/distributor.go:	if d.cfg.ShardByAllLabels {
pkg/distributor/distributor_test.go:			distributorCfg.ShardByAllLabels = true
pkg/distributor/distributor_test.go:		distributorCfg.ShardByAllLabels = cfg.shardByAllLabels
pkg/distributor/distributor_test.go:func TestShardByAllLabelsReturnsWrongResultsForUnsortedLabels(t *testing.T) {

just assume the value is true and organize the code accordingly

@tesla59
Copy link
Contributor Author

tesla59 commented Jun 18, 2024

@friedrichg should i remove the field from the config struct itself?

ShardByAllLabels bool `yaml:"shard_by_all_labels"`

Signed-off-by: tesla59 <nishant@heim.id>
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jun 19, 2024
@CharlieTLe
Copy link
Member

@friedrichg should i remove the field from the config struct itself?

ShardByAllLabels bool `yaml:"shard_by_all_labels"`

If this is removed, wouldn't this cause problems for config files that still reference this field?

@tesla59
Copy link
Contributor Author

tesla59 commented Jun 22, 2024

@friedrichg should i remove the field from the config struct itself?

ShardByAllLabels bool `yaml:"shard_by_all_labels"`

If this is removed, wouldn't this cause problems for config files that still reference this field?

Can we assume that the value is true and remove wherever its referenced?

Signed-off-by: tesla59 <nishant@heim.id>
Signed-off-by: tesla59 <nishant@heim.id>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 22, 2024
@CharlieTLe
Copy link
Member

CharlieTLe commented Jun 23, 2024

@friedrichg should i remove the field from the config struct itself?

ShardByAllLabels bool `yaml:"shard_by_all_labels"`

If this is removed, wouldn't this cause problems for config files that still reference this field?

Can we assume that the value is true and remove wherever its referenced?

I think the flag and field in the config struct should still exist, but marked as deprecated until it's fully removed. Perhaps the default should be set to true, and if someone is still setting it to false, then an error or warning should appear letting them know that the behavior is changing.

If we completely remove the field, people that are upgrading to this version will see an error like:

 error loading config from /config/cortex-config.yaml: Error parsing config file: yaml: unmarshal errors:
   line 21: field shard_by_all_labels not found in type distributor.Config

More info about the unmarshalling:

cortex/cmd/cortex/main.go

Lines 245 to 248 in 498635f

err = yaml.UnmarshalStrict(buf, cfg)
if err != nil {
return errors.Wrap(err, "Error parsing config file")
}

@friedrichg
Copy link
Member

pkg/ingester/limiter_test.go:228:167: too many arguments in call to NewLimiter
	have (*validation.Overrides, *ringCountMock, string, bool, int, bool, string)
	want (*validation.Overrides, RingCount, string, int, bool, string)

the test should compile @tesla59. A review is not worth for code that doesn't compile, I feel.

@tesla59
Copy link
Contributor Author

tesla59 commented Jun 27, 2024

Hey @friedrichg
Sorry for being inactive. I was busy with some work so couldn't get to it. I believe this needs some more discussion on how to proceed further

Also I'm not sure about how u got notification for review. I didn't ping u because I was still working on fixing the build

@friedrichg
Copy link
Member

friedrichg commented Jun 27, 2024

@tesla59 Just trying to guide the PR on the next step to follow. Thanks for your work so far!

@danielblando
Copy link
Contributor

We are in the process of releasing 1.18
Please rebase PR and change the changelog to master

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.

Deprecate -distributor.shard-by-all-labels
4 participants