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

disable index dedupe when rf > 1 and current or upcoming index type is boltdb-shipper #2206

Conversation

sandeepsukhani
Copy link
Contributor

@sandeepsukhani sandeepsukhani commented Jun 10, 2020

What this PR does / why we need it:
We use dedupe caches for chunks and seriesIDs to determine whether we already have written a chunk or a seriesID already to the store. Since boltdb-shipper only uploads boltdb files periodically to make the index available to all the other services, an ingester which first wrote a chunk or seriesID goes down and other ingesters didn't write them due to deduplication, we would be missing some logs in query responses until that ingester is down. This would be a problem even during rollouts.

This PR disables index deduplication and avoid using write dedupe cache(which dedupes seriesIDs) when replication factor > 1 and current or upcoming index type is boltdb-shipper.

Checklist

  • Documentation added
  • Tests updated

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2020

Codecov Report

Merging #2206 into master will increase coverage by 0.03%.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2206      +/-   ##
==========================================
+ Coverage   61.62%   61.65%   +0.03%     
==========================================
  Files         160      160              
  Lines       13577    13586       +9     
==========================================
+ Hits         8367     8377      +10     
+ Misses       4588     4586       -2     
- Partials      622      623       +1     
Impacted Files Coverage Δ
pkg/loki/modules.go 11.52% <61.53%> (+2.38%) ⬆️
pkg/querier/queryrange/downstreamer.go 95.87% <0.00%> (-2.07%) ⬇️
pkg/canary/comparator/comparator.go 80.24% <0.00%> (+2.41%) ⬆️

@slim-bean
Copy link
Collaborator

I was hoping we wouldn't need this if we could update cortex to fix the underlying issue?

@sandeepsukhani
Copy link
Contributor Author

I am not sure which issue we are talking about here. This is based on a change in Cortex that I did which helped with this.

@sandeepsukhani
Copy link
Contributor Author

Another note, this problem is specific to only boltdb shipper because all the other stores have strong consistency while boltdb shipper doesn't.

@slim-bean
Copy link
Collaborator

I will look at this closer, maybe it's the naming that confuses me.

We don't want to disable write de-duplication of chunks, we still want the chunks to be written to the cache.

Instead we just want to make sure when a chunk is already in the cache we still write the index entry.

This does get more complicated however if the filesystem is not shared, I will look closer at this.

@sandeepsukhani sandeepsukhani force-pushed the disable-write-dedupe-boltdb-shipper branch from 1fc5b02 to 66d68de Compare June 27, 2020 10:05
@sandeepsukhani
Copy link
Contributor Author

@slim-bean I have changed the flag in Cortex to allow disabling of just index deduplication. If enabled, it would still write just the index even if the chunk is already written.

@sandeepsukhani sandeepsukhani changed the title disable write dedupe when rf > 1 and current or upcoming index type is boltdb-shipper disable index dedupe when rf > 1 and current or upcoming index type is boltdb-shipper Jun 27, 2020
@sandeepsukhani sandeepsukhani force-pushed the disable-write-dedupe-boltdb-shipper branch from 66d68de to 1ed418b Compare July 24, 2020 11:05
@sandeepsukhani
Copy link
Contributor Author

@slim-bean let us merge this PR which has a helper function to check whether current or upcoming index store is of type boltdb-shipper. This would help with doing changes to #2166 that we discussed yesterday.

Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

LGTM!

@sandeepsukhani sandeepsukhani merged commit 807193d into grafana:master Jul 27, 2020
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