Skip to content

Conversation

@replay
Copy link
Contributor

@replay replay commented Apr 18, 2022

This removes the OldChunkDiskMapper which we added a few months ago.

At the time we added it because we needed a way to disable the chunk write queue and in upstream this wasn't possible. In the meantime upstream has been modified in such a way that the chunk write queue can also be disabled there by passing a size of 0, so we should make mimir-prometheus match upstream again.

Edit:

I updated this PR, the current description is now this one: #216 (comment)

@replay replay marked this pull request as draft April 18, 2022 17:39
@replay replay marked this pull request as ready for review April 18, 2022 18:21
@pracucci
Copy link
Collaborator

I prefer to keep the OldChunkDiskMapper until we've migrated our prod cells to the new one (progressively). Reason why OldChunkDiskMapper back in the days (even if it was already supported to disable the queue-based writer with a value 0) was to have a safer way to progressively rollout to the queue-based one

Copy link
Contributor

@jesusvazquez jesusvazquez left a comment

Choose a reason for hiding this comment

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

When I brought prometheus/main changes into mimir-prometheus I had to add this to a test so that the behaviour would be the same

opts.ChunkWriteQueueSize = 1 // We need to set this option so that we use the async queue. Upstream prometheus uses the queue directly.

That would probably conflict in future merges so lets not forget to leave it the same as in prometheus/main once we're done with this PR.

Signed-off-by: Mauro Stettler <[email protected]>
Signed-off-by: Mauro Stettler <[email protected]>
@replay replay changed the title remove old chunk disk mapper initialization and replace it with upstream add new chunk disk mapper from upstream and add option to use it Apr 19, 2022
replay added 2 commits April 19, 2022 20:15
Signed-off-by: Mauro Stettler <[email protected]>
@replay replay changed the title add new chunk disk mapper from upstream and add option to use it add option to use the new chunk disk mapper from upstream Apr 19, 2022
@replay
Copy link
Contributor Author

replay commented Apr 19, 2022

I prefer to keep the OldChunkDiskMapper until we've migrated our prod cells to the new one (progressively). Reason why OldChunkDiskMapper back in the days (even if it was already supported to disable the queue-based writer with a value 0) was to have a safer way to progressively rollout to the queue-based one

I updated this PR, now the only thing which it does is to add an option to use the new chunk disk mapper.
Note that this new option is independent of the chunk write queue size, that way we can first (slowly) roll out the new chunk disk mapper without activating the queue, if this works fine then we can remove the old chunk disk mapper. The primary goal of this is to make mimir-prometheus as similar to upstream as possible, the goal is not to roll out the chunk write queue again. In a later step we can consider re-activating the chunk write queue.
The default value of the new option is to continue using the old chunk disk mapper.

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM (no need to block on my comment)

Signed-off-by: Mauro Stettler <[email protected]>
@replay replay merged commit 64e6c17 into main Apr 25, 2022
@replay replay deleted the merge_upstream branch April 25, 2022 15:27
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.

4 participants