Skip to content

Allow 2h block conversion if it has no compact mark - #6865

Open
SungJin1212 wants to merge 1 commit into
cortexproject:masterfrom
SungJin1212:Allow-2h-nocompactmark-block-conversion
Open

SungJin1212 wants to merge 1 commit into
cortexproject:masterfrom
SungJin1212:Allow-2h-nocompactmark-block-conversion

Conversation

@SungJin1212

Copy link
Copy Markdown
Member

Allow the parquet converter to convert 2h blocks if the block has a no-compact.mark.json file.

Which issue(s) this PR fixes:
Fixes #6831

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@dosubot dosubot Bot added the storage/blocks Blocks storage engine label Jul 7, 2025
@SungJin1212
SungJin1212 requested a review from yeya24 July 7, 2025 02:49
@SungJin1212
SungJin1212 force-pushed the Allow-2h-nocompactmark-block-conversion branch from b3c1f38 to 528d1af Compare July 7, 2025 03:06

@yeya24 yeya24 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general, I think it looks good. Just some nits.

We should probably also introduce bucket cache for no compact marker file to reduce number of calls to object store

Comment thread pkg/parquetconverter/converter.go Resolved
@yeya24
yeya24 requested a review from alanprot July 7, 2025 05:32
@SungJin1212
SungJin1212 force-pushed the Allow-2h-nocompactmark-block-conversion branch 2 times, most recently from ce12528 to 369dbf3 Compare July 7, 2025 11:23
@SungJin1212
SungJin1212 requested a review from yeya24 July 9, 2025 00:50
@SungJin1212
SungJin1212 force-pushed the Allow-2h-nocompactmark-block-conversion branch 2 times, most recently from 54d9d99 to 7bb8c24 Compare July 9, 2025 05:39
@SungJin1212

Copy link
Copy Markdown
Member Author

@alanprot
Could you take a look?

@danielblando

Copy link
Copy Markdown
Contributor

Do we need this new config NoCompactMarkCheckAfter? Maybe one thing that we can do is if there is mint >= timeRange[1] or even dont have any. This files are not common and normally if they are there it doesnt go away that fast. We could just also add the cache and leave with it

@SungJin1212

SungJin1212 commented Jul 15, 2025 •

Copy link
Copy Markdown
Member Author

@yeya24
I also think it's better to convert the no-compact-mark blocks only if its mint is over timeRange[1] or timeRange[0] * constant. WDYT?

@sandy2008 sandy2008 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The approach looks right for #6831, but the age check that's meant to limit the extra object-store calls doesn't work as intended (inline).

Comment thread pkg/storage/parquet/util.go Outdated
// since these will never be compacted to 12h block.
// We check if the `no-compact-mark.json` file exists only for blocks
// after the noCompactMarkCheckAfter to reduce object storage calls.
if mint >= noCompactMarkCheckAfter {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

mint is a block timestamp in epoch milliseconds, while noCompactMarkCheckAfter is a duration (13h by default, so 46800000). This condition is therefore true for every real block, and the marker is checked for every unconverted 2h block on every conversion cycle, however recent the block is. I tried a 2h block that ended an hour ago: checkFunc is called and the block is converted. The tests only pass because they use timestamps near the epoch.

If the idea is to skip blocks younger than the threshold, it needs to compare against the current time, e.g. now - maxt >= noCompactMarkCheckAfter with now passed in so it stays testable. The blocks cleaner also calls this for every non-parquet block, so it adds to the Exists calls the bucket-cache suggestion above was about.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, I fixed it to the latest commit. Also, I changed it to use now - maxt >= timeRanges[1].

@SungJin1212
SungJin1212 force-pushed the Allow-2h-nocompactmark-block-conversion branch from 7bb8c24 to 74f83c6 Compare November 6, 2026 08:23
@SungJin1212
SungJin1212 requested a review from a team as a code owner November 6, 2026 08:23
Signed-off-by: Sungjin1212 <tjdwls1201@gmail.com>
@SungJin1212
SungJin1212 force-pushed the Allow-2h-nocompactmark-block-conversion branch from 74f83c6 to 51b3154 Compare November 6, 2026 09:43
@SungJin1212

Copy link
Copy Markdown
Member Author

@yeya24 @danielblando
Would you help review this PR?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L storage/blocks Blocks storage engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parquet Converter should convert 2h block with no compact marker

4 participants