Parquet add no convert marker

[siddarth2810]

What this PR does:
If a TSDB block exceeds a configurable threshold of distinct label names, the converter writes a parquet-no-convert-mark.json marker and skips the block.

• Added no-convert marker with read/write logic
• Added parquet-converter.max-block-label-names limit

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

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]
• docs/configuration/v1-guarantees.md updated if this PR introduces experimental flags

Notes from the Previous PR review

• Removed the incorrect log under cortex_parquet.ValidConverterMarkVersion
• Embedded LabelNamesCount and Threshold into Reason field
• Added buffer for system columns and generated data columns
• Cleaned up skipped-block metrics when deleting tenant metrics

[SungJin1212]

1. If noConvertMark is upgraded (v1 -> v2), is it correct that the noConvertMark file is overwritten to v2?
2. When the user increases this limit (1000 -> 2000), I think we should convert the previous unconverted blocks. (ex. 1500)
   ㄴ We can track the current applied limit in noConvertMark and utilize it.

[siddarth2810]

There is only v1 version right now. I'm not quite sure in which cases the noConvertMark would be upgraded.

My implementation is based on comparing the current limit and label count. By tracking the applied limit, do you mean comparing the limits ?

Like, if current limit > old noConvertMark limit -> Retry conversion

[SungJin1212]

When the configured limit changes, I think we should re-evaluate the block against the new limit, instead of unconditionally skipping it just because a noConvertMark already exists.

[SungJin1212]

wdyt?

Suggested change

	if cortex_parquet.ValidNoConvertMarkVersion(noConvertMark.Version) && noConvertMark.ShouldSkipBlock(maxBlockLabelNames) {
	level.Debug(logger).Log("msg", "skipping block, no-convert marker already exists", "block", b.ULID.String())
	c.metrics.skippedBlocks.WithLabelValues(userID, cortex_parquet.NoConvertReasonMarkerExists).Inc()
	continue
	if cortex_parquet.ValidNoConvertMarkVersion(noConvertMark.Version) {
	level.Debug(logger).Log("msg", "skipping block, no-convert marker already exists", "block", b.ULID.String())
	c.metrics.skippedBlocks.WithLabelValues(userID, cortex_parquet.NoConvertReasonMarkerExists).Inc()
	continue
	}
	if noConvertMark.ShouldSkipBlock(maxBlockLabelNames) {
	level.Debug(logger).Log(
	"msg", "skipping block because label count still exceeds current limit",
	"block", b.ULID.String(),
	"label_names_count", noConvertMark.LabelNamesCount,
	"current_limit", maxBlockLabelNames,
	)
	c.metrics.skippedBlocks.WithLabelValues(userID, cortex_parquet.NoConvertReasonTooManyLabels).Inc()
	continue
	}
	}

[yeya24]

If we change to this way, it means that if the no converter marker already exists we won't re-evaluate the no convert marker check?

[SungJin1212]

ah, I mean

if cortex_parquet.ValidNoConvertMarkVersion(noConvertMark.Version) {
	if noConvertMark.Reason != cortex_parquet.NoConvertReasonTooManyLabels {
		level.Debug(logger).Log(
			"msg", "skipping block, no-convert marker already exists",
			"block", b.ULID.String(),
		)
		c.metrics.skippedBlocks.WithLabelValues(userID, cortex_parquet.NoConvertReasonMarkerExists).Inc()
		continue
	}

	if noConvertMark.ShouldSkipBlock(maxBlockLabelNames) {
		level.Debug(logger).Log(
			"msg", "skipping block because label count still exceeds current limit",
			"block", b.ULID.String(),
			"label_names_count", noConvertMark.LabelNamesCount,
			"current_limit", maxBlockLabelNames,
		)
		c.metrics.skippedBlocks.WithLabelValues(userID, cortex_parquet.NoConvertReasonTooManyLabels).Inc()
		continue
	}
}

[siddarth2810]

Yep, if we change this way, then every no converter marker will be skipped immediately. ShouldSkipBlock(maxBlockLabelNames) is never be checked

[siddarth2810]

ah, I mean

if cortex_parquet.ValidNoConvertMarkVersion(noConvertMark.Version) {
	if noConvertMark.Reason != cortex_parquet.NoConvertReasonTooManyLabels {
		level.Debug(logger).Log(
			"msg", "skipping block, no-convert marker already exists",
			"block", b.ULID.String(),
		)
		c.metrics.skippedBlocks.WithLabelValues(userID, cortex_parquet.NoConvertReasonMarkerExists).Inc()
		continue
	}

	if noConvertMark.ShouldSkipBlock(maxBlockLabelNames) {
		level.Debug(logger).Log(
			"msg", "skipping block because label count still exceeds current limit",
			"block", b.ULID.String(),
			"label_names_count", noConvertMark.LabelNamesCount,
			"current_limit", maxBlockLabelNames,
		)
		c.metrics.skippedBlocks.WithLabelValues(userID, cortex_parquet.NoConvertReasonTooManyLabels).Inc()
		continue
	}
}

This makes sense. I made the same check here:

func (m NoConvertMark) ShouldSkipBlock(currentMaxBlockLabelNamesLimit int) bool {
	// Manual no-convert marks are not tied to the label-name limit
	if m.Reason != NoConvertReasonTooManyLabels {
		return true
	}

But, I think writing in the way you suggested make it easier to understand on the first read.

[SungJin1212]

lgtm

[friedrichg]

LGTM

[SungJin1212]

would be nice if we use the parquetConverterDataColumnDuration in baseConverterOptions.

baseConverterOptions := []convert.ConvertOption{
		convert.WithColDuration(parquetConverterDataColumnDuration),
		convert.WithRowGroupSize(cfg.MaxRowsPerRowGroup),
	}

[SungJin1212]

@siddarth2810
Can you get the rebase?

[siddarth2810]

Done, sorry for the delay

[SungJin1212]

I think this test cannot execute in CI.
change to requires_docker or something?

[siddarth2810]

Done and solved small merge conflicts. Thank you :)