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

Remove old chunk mapper. #311

Merged
merged 4 commits into from
Aug 18, 2022
Merged

Remove old chunk mapper. #311

merged 4 commits into from
Aug 18, 2022

Conversation

pstibrany
Copy link
Member

This PR removes OldChunkDiskMapper

@pstibrany
Copy link
Member Author

pstibrany commented Aug 8, 2022

@codesome I've removed old chunk mapper in this PR, but now TestOOOCompaction and TestOOOCompactionFailure are failing. It turns out that old chunk mapper was used by default instead of new chunk mapper (which is also in upstream Prometheus).

Would you please take a look at the failing tests? There seem to be small difference between old and new chunk mapper. I'll try to look at it as well, but you may have better idea about the differences between old and new chunk mapper (perhaps just the fact that new one is async is enough change to cause this).

Failures: https://github.com/grafana/mimir-prometheus/runs/7728655169?check_suite_focus=true

@pstibrany
Copy link
Member Author

@codesome I've removed old chunk mapper in this PR, but now TestOOOCompaction and TestOOOCompactionFailure are failing. It turns out that old chunk mapper was used by default instead of new chunk mapper (which is also in upstream Prometheus).

Would you please take a look at the failing tests? There seem to be small difference between old and new chunk mapper. I'll try to look at it as well, but you may have better idea about the differences between old and new chunk mapper (perhaps just the fact that new one is async is enough change to cause this).

Failures: https://github.com/grafana/mimir-prometheus/runs/7728655169?check_suite_focus=true

Fix is likely available in prometheus/prometheus#11075.

@codesome
Copy link
Member

prometheus/prometheus#11075 is same as the one in mimir-prometheus, and tests might fail for async queue there as well I think. In Prometheus the queue is disabled by default I believe. I will take a look at those tests in coming days.

@codesome
Copy link
Member

The last commit should fix the tests. I will merge on green (minus the unrelated lint failure)

@pstibrany
Copy link
Member Author

Thanks @codesome, much appreciated! I can take care of updating mimir-prometheus in Mimir as a next step.

@codesome
Copy link
Member

Another test failure because of some sorting issue. On it.

pstibrany and others added 4 commits August 18, 2022 13:50
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
@codesome codesome enabled auto-merge (squash) August 18, 2022 08:24
@codesome codesome merged commit acd4a8a into main Aug 18, 2022
@pstibrany pstibrany deleted the remove-old-chunk-mapper2 branch November 28, 2022 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants