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

use tsdb config for initializing index gateway client #6267

Merged
merged 2 commits into from
May 31, 2022
Merged

use tsdb config for initializing index gateway client #6267

merged 2 commits into from
May 31, 2022

Conversation

sandeepsukhani
Copy link
Contributor

What this PR does / why we need it:
We were temporarily using boltdb-shipper config for initializing the index gateway client. This PR fixes it by doing relevant updates.

@sandeepsukhani sandeepsukhani requested a review from a team as a code owner May 30, 2022 11:37
@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%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.1%

DylanGuedes
DylanGuedes approved these changes May 30, 2022
View reviewed changes
Copy link
Contributor

@DylanGuedes DylanGuedes left a comment

Choose a reason for hiding this comment

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

lgtm!

just a note: maybe in the future we can tweak the TSDBShipperConfig and the BoltDBShipperConfig to somehow be shared, to avoid the duplications (like the ones in modules.go)

@sandeepsukhani
Copy link
Contributor Author

lgtm!

just a note: maybe in the future we can tweak the TSDBShipperConfig and the BoltDBShipperConfig to somehow be shared, to avoid the duplications (like the ones in modules.go)

yeah, that is the plan. We are temporarily going with duplicate configs. I would love to move it to a shipper config which would be shared by boltdb-shipper and tsdb but that can't be done cleanly without breaking backwards compatibility.

kavirajk
kavirajk approved these changes May 31, 2022
View reviewed changes
Copy link
Contributor

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

LGTM!. One minor nit.

pkg/storage/store.go Outdated Show resolved Hide resolved
@sandeepsukhani @kavirajk
review feedback
1ca2c23 
Co-authored-by: Kaviraj Kanagaraj <kavirajkanagaraj@gmail.com>
@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%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.1%

@sandeepsukhani sandeepsukhani merged commit 7369b37 into grafana:main May 31, 2022
This was referenced Jun 28, 2022
Closed
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kavirajk kavirajk kavirajk approved these changes

@DylanGuedes DylanGuedes DylanGuedes approved these changes

@owen-d owen-d Awaiting requested review from owen-d

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sandeepsukhani @grafanabot @kavirajk @DylanGuedes