compact/planner: fix issue 6775 #7334
Conversation
It doesn't make sense to vertically compact downsampled blocks so mark them with the no compact marker if downsampled blocks were detected in the plan. Seems like the Planner is the best place for this logic - I just repeated the previous pattern with the large index file filter. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
There was a problem hiding this comment.
Instead of creating a new planner and excluding downsampled blocks from planning time, why we cannot filter out downsampled blocks from metasByMinTime when we pass blocks to the block?
I understand that compaction should only happen for raw blocks. Is there a usecase to compact downsampled blocks or compact downsampled blocks with raw blocks? If we do this, downsampled blocks will turn into raw blocks again, and we need to downsampled it again? I feel this is not ideal.
I am inclined to agree but I am afraid that there will be some weird use-case out there that depends on compacting downsampled blocks. Maybe we could go with the current change as-is for the next release and then in $nextrelease+1 I will do as you've suggested? |
|
I deployed this in prod and it works. 😄 |
|
I also deployed this in prod and it's working like a charm 🥳 |
Let me think about this more. Downsampled blocks can be compacted with downsampled blocks with the same resolution only since they are in the same compaction group. It sounds possible to happen in the current planner creating a plan compacting downsampled blocks together. But I can only see blocks getting compacted into a raw block and the raw block getting downsampled again. When two downsampled blocks overlap with each other, then it is probably valid to do a vertical compaction and merge them into 1 block. Is it possible to create a downsampled block directly rather than compacting a raw block first then get it downsampled again? |
Unless two downsampled and vertically overlapping blocks are exactly the same, it is impossible to do vertical compaction on them. For example, it would be impossible to calculate I agree with you and I'm not 100% certain how this could happen but Filip had an idea that it is maybe related to the "index size too big" filter which could cause blocks to have "gaps" in them due to some blocks being filtered from compaction. Then, the downsampling part probably picks up those blocks that had been left out from compaction. And we end up with this situation. |
We can merge this pr first and think about not compacting downsampled blocks at all in next release. |
|
@GiedriusS Let's add changelog? |
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
|
@yeya24 added |
It doesn't make sense to vertically compact downsampled blocks so mark them with the no compact marker if downsampled blocks were detected in the plan. Seems like the Planner is the best place for this logic - I just repeated the previous pattern with the large index file filter.
Closes #6775 .