Skip to content

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Sep 2, 2025

We should not enable the TSDB codec for _recovery_source and _recovery_source_size fields, as their doc_values can be filtered out in RecoverySourcePruneMergePolicy, leading to inconsistencies between merge stats and actual values.

Closes #133875
Closes #133993

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

@dnhatn dnhatn force-pushed the disable-dv-for-recovery-source branch from 151a7c9 to 047e8a0 Compare September 2, 2025 17:18
@martijnvg martijnvg added the test-release Trigger CI checks against release build label Sep 2, 2025
@martijnvg
Copy link
Member

Added test-release label given that this issue only occurs with release tests.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Given that the recovery_source_* fields get trimmed away, it makes sense to use default doc values codec for these fields. The encoding techniques that the tsdb codec is doing is likely not worth the effort, given that this fields get trimmed away. I think the storage difference should be negligible.

LGTM 👍

// TODO: should we just allow all fields to use tsdb doc values codec?
// Values of these doc_values fields can be filtered out in RecoverySourcePruneMergePolicy,
// which leads to inconsistencies between merge stats and actual values.
if (SourceFieldMapper.RECOVERY_SOURCE_SIZE_NAME.equals(fieldName) || SourceFieldMapper.RECOVERY_SOURCE_NAME.equals(fieldName)) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also update PerFieldMapperCodecTests to test for this new logic?

martijnvg pushed a commit to martijnvg/elasticsearch that referenced this pull request Sep 2, 2025
@dnhatn
Copy link
Member Author

dnhatn commented Sep 2, 2025

This change will be included in #134008

@dnhatn dnhatn closed this Sep 2, 2025
@dnhatn dnhatn deleted the disable-dv-for-recovery-source branch September 2, 2025 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] LogsIndexingIT testRouteOnSortFields failing [CI] LogsIndexingIT failing, DirectMonotonicWriter reports wrong number of values added

3 participants