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.

Sign up for GitHub

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

Jump to bottom

[release-2.7.x] Add single compactor http client for delete and gennumber clients #7607

Merged
merged 1 commit into from
Nov 30, 2022
Merged

[release-2.7.x] Add single compactor http client for delete and gennumber clients #7607

merged 1 commit into from
Nov 30, 2022
+80 −55

Conversation

periklis
Copy link
Collaborator

@periklis periklis commented Nov 7, 2022

What this PR does / why we need it:
Manual backport of #7453 into release-2.7.x.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@periklis periklis requested a review from a team as a code owner November 7, 2022 10:15
@periklis periklis self-assigned this Nov 7, 2022
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

-           ingester	-0.2%
-        distributor	-2.4%
+            querier	0%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@periklis periklis closed this Nov 9, 2022
@periklis periklis reopened this Nov 11, 2022
@periklis periklis force-pushed the backport-7453-to-release-2.7.x branch from 30a4933 to 38f7e1c Compare November 11, 2022 13:48
@pull-request-size pull-request-size bot added size/M and removed size/L labels Nov 11, 2022
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
-        distributor	-0.4%
+            querier	0%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0.2%
+           chunkenc	0%
-              logql	-0.1%
+               loki	0.5%

@periklis periklis mentioned this pull request Nov 17, 2022
Closed
5 tasks
chaudum
chaudum reviewed Nov 28, 2022
View reviewed changes
pkg/loki/loki.go Outdated
@@ -74,7 +74,7 @@ type Config struct {
InternalServer internalserver.Config `yaml:"internal_server,omitempty"`
Distributor distributor.Config `yaml:"distributor,omitempty"`
Querier querier.Config `yaml:"querier,omitempty"`
DeleteClient deletion.Config `yaml:"delete_client,omitempty"`
CompactorClient compactor.ClientConfig `yaml:"compactor_client,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a breaking change when the config key changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only for v2.7.0 since we released it without this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but we shouldn't backport breaking changes. People upgrading from 2.7.0 to 2.7.1 would not expect a breaking change.

Can we get this fix into the 2.7 release branch without being a breaking change, e.g. not changing the config key (and CLI argument name). That should work, because the configs are compatible, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Working on this right a way

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@chaudum Change this, PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good!

@periklis periklis force-pushed the backport-7453-to-release-2.7.x branch from 38f7e1c to bddaa48 Compare November 30, 2022 10:20
@periklis periklis force-pushed the backport-7453-to-release-2.7.x branch from bddaa48 to 3aba37c Compare November 30, 2022 10:20
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 30, 2022
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

-           ingester	-0.5%
-        distributor	-0.4%
-            querier	-0.1%
- querier/queryrange	-0.7%
+               iter	0%
+            storage	0.2%
+           chunkenc	0%
+              logql	1.7%
+               loki	0.5%

@periklis periklis merged commit 2e0148a into grafana:release-2.7.x Nov 30, 2022
periklis added a commit to periklis/loki that referenced this pull request Nov 30, 2022
@periklis
[release-2.7.x] Add single compactor http client for delete and gennu…
58c4f80 
…mber clients (grafana#7607)
@periklis periklis mentioned this pull request Nov 30, 2022
Merged
5 tasks
@trevorwhitney trevorwhitney added the backport release-2.7.x add to a PR to backport it into release 2.7.x label Dec 7, 2022
@grafanabot
Copy link
Collaborator

Hello @trevorwhitney!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@trevorwhitney trevorwhitney removed the backport release-2.7.x add to a PR to backport it into release 2.7.x label Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chaudum chaudum chaudum approved these changes

@trevorwhitney trevorwhitney Awaiting requested review from trevorwhitney

Assignees

@periklis periklis

Labels
missing-labels size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@periklis @grafanabot @chaudum @trevorwhitney