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

make index shipper read path handle range of tables by type of index #6304

Merged

Conversation

sandeepsukhani
Copy link
Contributor

What this PR does / why we need it:
We run one indexshipper instance for each boltdb-shipper and tsdb instance. While initializing indexshipper, both the stores pass an index specific OpenIndexFileFunc to help indexshipper open the index files without having to make it aware of the file types or the way to interpret them.

If Loki is using both boltdb-shipper and tsdb for the index store and has the same local and/or object storage path configured for storing the index files, it fails to run because the generic indexshipper running for both the store types tries to open both types of index files using the same OpenIndexFileFunc.

This PR makes indexshipper accept a range of table numbers to handle only specific tables i.e it would sync and load-on-startup only specific tables. The table number ranges are calculated based on the configured schema start/end date and the index period of both the index types.

Checklist

  • Tests updated

@sandeepsukhani sandeepsukhani requested a review from owen-d June 3, 2022 11:30
@sandeepsukhani sandeepsukhani requested a review from a team as a code owner June 3, 2022 11:30
@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.3%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@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.3%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

This looks great. One thing I think we should add is support for skipping files within a table. For instance, consider two period configs (I know we're hardcoding period to 24h now, but it's a configurable field and should/can be configurable in the future IMO):

  1. boltdb, period=24h
  2. tsdb, period=48h

It's possible for an earlier timestamp (i.e. 72h) to have the same bucket as a later one (144h) because 72h / 24h = 3 and 144h / 48h = 3.

Approving in the meantime :)

@sandeepsukhani
Copy link
Contributor Author

One thing I think we should add is support for skipping files within a table.

Yeah, I think we will have to keep the file name format unique to be able to distinguish between different index file types.

@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.4%
+           chunkenc	0%
+              logql	0%
+               loki	0%

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