Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release-2.9.x] Pin chunk and index format to schema version. #10333

Merged
merged 1 commit into from Aug 24, 2023

Conversation

grafanabot
Copy link
Collaborator

Backport bbfb13c from #10213


We pin all three Chunk, HeadBlock and TSDB Version to schema version in period config.

This is the following mapping (after being discussed with @owen-d and @sandeepsukhani )

v12 (current existing schema) - ChunkFormatV3 (UnorderedHeadBlock) + TSDBv2
v13 (introducing new schema) - ChunkFormatV4 (UnorderedWithNonIndexedLabelsHeadBlockFmt) + TSDBv3

Note the new schema v13 supports the latest chunk and index format.

NOTES for Reviewer

  1. General approach is we removed the idea of index.LiveFormat, chunkenc.DefaultChunkFormat and chunkenc.DefaultHeadBlockFmt and made following two changes. These variables were used before to tie chunk and tsdb formats specific to Loki versions. This PR remove that coupling and pin these formats to schema version instead.

    1. These variables were replaced with explicit chunk and index formats within those packages (and it's tests)
    2. If these variables were used outside it's own packages say by ingester, compactor, etc. Then we extract correct chunk and index versions from the schema config.
  2. Add two methods to periodConfig. (1) ChunkFormat() returning chunk and head format tied to schema (2) TSDBFormat() returning tsdb format tied to schema.

  3. Other ideas I thought of doing but didn't end up doing is make ChunkFormat and IndexFormat as separate type (rather than byte and int currently. Similar to HeadBlockFmt type). But didnt' do it eventually to keep the PR small and don't want to complicate with lots of changes.

  4. Moved couple of test cases from chunkenc to config package, because the test case was actually testing methods on schemaconfig and it was creating cycling dependencies.

We pin all three `Chunk`, `HeadBlock` and `TSDB` Version to `schema`
version in period config.

This is the following mapping (after being discussed with @owen-d and
@sandeepsukhani )

`v12` (current existing schema) - ChunkFormatV3 (UnorderedHeadBlock) +
TSDBv2
`v13` (introducing new schema) - ChunkFormatV4
(UnorderedWithNonIndexedLabelsHeadBlockFmt) + TSDBv3

Note the new schema `v13` supports the latest chunk and index format.

**NOTES for Reviewer**

1. General approach is we removed the idea of `index.LiveFormat`,
`chunkenc.DefaultChunkFormat` and `chunkenc.DefaultHeadBlockFmt` and
made following two changes. These variables were used before to tie
chunk and tsdb formats specific to Loki versions. This PR remove that
coupling and pin these formats to `schema` version instead.

1. These variables were replaced with explicit chunk and index formats
within those packages (and it's tests)
2. If these variables were used outside it's own packages say by
ingester, compactor, etc. Then we extract correct chunk and index
versions from the `schema` config.

2. Add two methods to `periodConfig`. (1) `ChunkFormat()` returning
chunk and head format tied to schema (2) `TSDBFormat()` returning tsdb
format tied to schema.

2. Other ideas I thought of doing but didn't end up doing is make
`ChunkFormat` and `IndexFormat` as separate type (rather than `byte` and
`int` currently. Similar to `HeadBlockFmt` type). But didnt' do it
eventually to keep the PR small and don't want to complicate with lots
of changes.

4. Moved couple of test cases from `chunkenc` to `config` package,
because the test case was actually testing methods on `schemaconfig` and
it was creating cycling dependencies.

---------

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
(cherry picked from commit bbfb13c)
@kavirajk kavirajk merged commit 70bfc98 into release-2.9.x Aug 24, 2023
10 checks passed
@kavirajk kavirajk deleted the backport-10213-to-release-2.9.x branch August 24, 2023 08:14
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.

None yet

2 participants