Skip to content

Conversation

@kkrik-es
Copy link
Contributor

@kkrik-es kkrik-es commented Nov 17, 2025

Introduces a new version of the existing tsdb doc values format that allows configuring the block size, adding 512 to the existing size of 128. The block size is included it codec metadata, so that blocks written with the larger setting will be read with this as well. This provides flexibility for configuring tsdb and logsdb indices differently, using the same codec version.

While this change bumps up the version, it should still be a no-op (other than writing and reading the block size to the metadata). The larger block size will be wired for use in tsdb indices in a follow-up change.

Related to #136307

@kkrik-es kkrik-es changed the title Add accessors for block size constants in tsdb codec Introduce large block doc values format Nov 18, 2025
@kkrik-es kkrik-es added the test-full-bwc Trigger full BWC version matrix tests label Nov 18, 2025
@kkrik-es kkrik-es marked this pull request as ready for review November 18, 2025 20:10
@elasticsearchmachine
Copy link
Collaborator

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

@kkrik-es kkrik-es requested a review from romseygeek November 19, 2025 16:18
kkrik-es and others added 12 commits November 19, 2025 18:56
# Conflicts:
#	server/src/main/java/org/elasticsearch/index/codec/tsdb/es819/ES819TSDBDocValuesConsumer.java
#	server/src/main/java/org/elasticsearch/index/codec/tsdb/es819/ES819TSDBDocValuesFormat.java
# Conflicts:
#	server/src/main/java/org/elasticsearch/index/codec/tsdb/es819/ES819TSDBDocValuesFormat.java
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.

Thanks Kostas! This looks good. I left a few minor comments.

}

public static int randomNumericBlockSize() {
return random().nextBoolean() ? ES819TSDBDocValuesFormat.NUMERIC_LARGE_BLOCK_SHIFT : ES819TSDBDocValuesFormat.NUMERIC_BLOCK_SHIFT;
Copy link
Member

Choose a reason for hiding this comment

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

Let's test with more blockshifts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an assert to check that only these two values are allowed. We can add more as needed.


// read in the entries from the metadata file.
int version = -1;
int blockShift = numericBlockShift;
Copy link
Member

Choose a reason for hiding this comment

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

Should numericBlockShift always be ES819TSDBDocValuesFormat.NUMERIC_BLOCK_SHIFT?

Copy link
Contributor Author

@kkrik-es kkrik-es Nov 27, 2025

Choose a reason for hiding this comment

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

Good catch! No need to pass the arg, indeed.

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.

LGTM 👍

@kkrik-es kkrik-es merged commit 30f327e into elastic:main Nov 28, 2025
27 of 30 checks passed
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.

4 participants