Fix: getting rid of EncOOOXOR from chunk encoding by mabhi · Pull Request #12111 · prometheus/prometheus

mabhi · 2023-03-09T06:28:47Z

Explanation

Getting rid of EncOOOXOR from chunk encoding, as this is not an encoding but a flag to mark OOO encoded chunks

Related Issue:

Fixes #11835

Proposed Changes

Refactor the code to get rid of OOOXORChunk struct
Enhance the ChunkDiskMapper with the responsibility to verify for OOOChunks, apply OOO header and restore original encoding
The ChunkDiskMapper uses the first bit to know if the chunk is from out-of-order or not, and uses the last 7 bits of the byte to get the chunk encoding, by assuming that the chunk encoding won't cross 7 bits anytime in near/far future.
During chunk writes either directly or via job queue sending the isOOO flag to persist the extra 1st bit and that would be used during reads to check for OOO chunks and restore the original encoding for further processing.

Proof Manifests

jesusvazquez

So far the work looks good 💪

I left one nitpick about the naming of a couple of functions.

The most important comment is the one in the head where the head is still aware of ooo chunk encodings. I think we can fix this refactoring the function by adding a boolean saying if the chunk is ooo but lets wait for @codesome 's comments.

Great work!

jesusvazquez · 2023-03-12T13:05:32Z

tsdb/chunks/head_chunks.go

+	RestoreOutOfOrderMask = 0b01111111
+)
+
+func (cdm *ChunkDiskMapper) OutOfOrderEncoding(sourceEncoding chunkenc.Encoding) chunkenc.Encoding {


I would rename this to ApplyOutOfOrderEncoding or something like that since its actually applying a mask over the giving encoding.

Ok, will rename it as suggested.

ApplyOutOfOrderMask is my suggestion. We are not changing encoding here. Reminder: we are not considering this bit as a part of encoding. For ChunkDiskMapper, only the last 7 bits are the encoding.

jesusvazquez · 2023-03-12T13:06:25Z

tsdb/chunks/head_chunks.go

+	return (e & OutOfOrderMask) != 0
+}
+
+func (cdm *ChunkDiskMapper) OutOfOrderDecoding(sourceEncoding chunkenc.Encoding) chunkenc.Encoding {


Same as above, we're applying a mask to decode

Ok, will rename it as suggested.

After getting rid of RestoreOutOfOrderMask as suggested, this can be renamed as UnapplyOutOfOrderMask.

jesusvazquez · 2023-03-12T13:14:21Z

tsdb/head.go

+		isOOO := h.chunkDiskMapper.IsOutOfOrderChunk(encoding)
+		// flip the first bit to restore the original encoding
+		if isOOO {
+			encoding = h.chunkDiskMapper.OutOfOrderDecoding(encoding)


Hm I wonder what could be done here, it seems here the head is aware of OOO chunk encodings and this is precisely what we wanted to hide in the chunk disk mapper.

Maybe we could change the function to return a boolean saying if its OOO instead of the encoding and then if its OOO we add it to oooMmappedChunks and if its not we add it to mmappedChunks

What do you think @codesome

@jesusvazquez Can you point out as to which function you want to return bool.
The current function is part of the load chunks from initial read. I have tried to put all the logic in CDM. Do you want me to pass this bool as part of the callback function inside the IterateAllChunks ?

Right! I believe I added the encoding to that function when we added the OOO chunks. If you notice the usages I think it should be fairly easy to change.

prometheus/tsdb/chunks/head_chunks.go

Line 765 in 3330d85

func (cdm *ChunkDiskMapper) IterateAllChunks(f func(seriesRef HeadSeriesRef, chunkRef ChunkDiskMapperRef, mint, maxt int64, numSamples uint16, encoding chunkenc.Encoding) error) (err error) {

I would change that signature to something like
func (cdm *ChunkDiskMapper) IterateAllChunks(f func(seriesRef HeadSeriesRef, chunkRef ChunkDiskMapperRef, mint, maxt int64, numSamples uint16, isOOO bool) error) (err error) {

That will force you to refactor the call in

prometheus/tsdb/head.go

Line 737 in 1f0cc09

if err := h.chunkDiskMapper.IterateAllChunks(func(seriesRef chunks.HeadSeriesRef, chunkRef chunks.ChunkDiskMapperRef, mint, maxt int64, numSamples uint16, encoding chunkenc.Encoding) error {

and remove the information about encodings in the head.

+1 with what @jesusvazquez said. Let's change the function signature to return the isOOO boolean such that head doesn't need to infer it from the chunk encoding. The encoding coming out of ChunkDiskMapper here should have that ooo bit removed.

codesome · 2023-03-13T10:59:43Z

tsdb/chunks/head_chunks.go

+	OutOfOrderMask        = 0b10000000
+	RestoreOutOfOrderMask = 0b01111111


We can use a single variable and get the RestoreOutOfOrderMask as ^OutOfOrderMask (note the uint8, else it can be buggy) https://goplay.tools/snippet/dCb5_qkFCbL

Suggested change

OutOfOrderMask = 0b10000000

RestoreOutOfOrderMask = 0b01111111

OutOfOrderMask = uint8(0b10000000)

codesome · 2023-03-13T11:00:42Z

tsdb/chunks/head_chunks.go

 	chkEnc := mmapFile.byteSlice.Range(chkStart, chkStart+ChunkEncodingSize)[0]
-
+	sourceChkEnc := chunkenc.Encoding(chkEnc)
+	if cdm.IsOutOfOrderChunk(sourceChkEnc) {


You don't need this if check. You can simply just apply the bit mask everytime.

codesome · 2023-03-13T11:04:54Z

tsdb/chunks/head_chunks.go

+	RestoreOutOfOrderMask = 0b01111111
+)
+
+func (cdm *ChunkDiskMapper) OutOfOrderEncoding(sourceEncoding chunkenc.Encoding) chunkenc.Encoding {


ApplyOutOfOrderMask is my suggestion. We are not changing encoding here. Reminder: we are not considering this bit as a part of encoding. For ChunkDiskMapper, only the last 7 bits are the encoding.

codesome · 2023-03-13T11:05:18Z

tsdb/chunks/head_chunks.go

+	return (e & OutOfOrderMask) != 0
+}
+
+func (cdm *ChunkDiskMapper) OutOfOrderDecoding(sourceEncoding chunkenc.Encoding) chunkenc.Encoding {


After getting rid of RestoreOutOfOrderMask as suggested, this can be renamed as UnapplyOutOfOrderMask.

jesusvazquez

LGTM 💪 💪

mabhi · 2023-03-15T05:03:27Z

@codesome / @jesusvazquez any other changes need in this PR. Do let me know.

codesome

Last nits

codesome · 2023-03-15T11:20:26Z

tsdb/chunks/head_chunks.go

+// Chunk encodings for out-of-order chunks.
+// These encodings must be only used by the Head block for its internal bookkeeping.
+const (
+	OutOfOrderMask = 0b10000000


This should be uint8 to begin with and don't need typecasting elsewhere, else it can lead buggy code if not used correctly.

Suggested change

OutOfOrderMask = 0b10000000

OutOfOrderMask = uint8(0b10000000)

codesome · 2023-03-15T11:23:44Z

tsdb/chunks/head_chunks.go

 	chkEnc := mmapFile.byteSlice.Range(chkStart, chkStart+ChunkEncodingSize)[0]
-
+	sourceChkEnc := chunkenc.Encoding(chkEnc)
+	// update the chunk encoding by restoring the original encoding, if needed


Other than the reword, in Prometheus we start comments with capital letters where applicable and end with a full stop.

Suggested change

// update the chunk encoding by restoring the original encoding, if needed

// Extract the encoding from the byte. ChunkDiskMapper uses only the last 7 bits for the encoding.

codesome · 2023-03-15T11:25:03Z

tsdb/chunks/head_chunks.go

+			// flip the first bit to restore the original encoding
+			if isOOO {
+				chkEnc = cdm.UnapplyOutOfOrderMask(chkEnc)
+			}


Same reword as the other comment + don't need the if block.

Suggested change

// flip the first bit to restore the original encoding

if isOOO {

chkEnc = cdm.UnapplyOutOfOrderMask(chkEnc)

}

// Extract the encoding from the byte. ChunkDiskMapper uses only the last 7 bits for the encoding.

chkEnc = cdm.UnapplyOutOfOrderMask(chkEnc)

codesome · 2023-03-15T11:25:52Z

tsdb/chunks/head_chunks.go

+	return (e & OutOfOrderMask) != 0
+}
+
+func (cdm *ChunkDiskMapper) UnapplyOutOfOrderMask(sourceEncoding chunkenc.Encoding) chunkenc.Encoding {


On another thought, I think this is better. Making it less specific to OOO such that it can be extended later if needed.

Suggested change

func (cdm *ChunkDiskMapper) UnapplyOutOfOrderMask(sourceEncoding chunkenc.Encoding) chunkenc.Encoding {

func (cdm *ChunkDiskMapper) RemoveMasks(sourceEncoding chunkenc.Encoding) chunkenc.Encoding {

Signed-off-by: mabhi <[email protected]>

…O encoding logic Signed-off-by: mabhi <[email protected]>

Signed-off-by: mabhi <[email protected]>

codesome

Last comment and we can merge. LGTM!

tsdb/chunks/head_chunks.go

Signed-off-by: mabhi <[email protected]>

mabhi force-pushed the remove-encoooxor branch from dfa4fcf to 1aa88c6 Compare March 10, 2023 11:05

mabhi marked this pull request as ready for review March 10, 2023 14:07

mabhi requested a review from codesome as a code owner March 10, 2023 14:07

jesusvazquez reviewed Mar 12, 2023

View reviewed changes

codesome reviewed Mar 13, 2023

View reviewed changes

jesusvazquez approved these changes Mar 13, 2023

View reviewed changes

codesome reviewed Mar 15, 2023

View reviewed changes

mabhi added 4 commits March 15, 2023 18:21

Fix: getting rid of EncOOOXOR from chunk encoding

71c4fd8

Signed-off-by: mabhi <[email protected]>

addressed changes w.r.t method naming and make head unaware of the OO…

afdc1d5

…O encoding logic Signed-off-by: mabhi <[email protected]>

addressed refactoring

33393f0

Signed-off-by: mabhi <[email protected]>

addressing refactoring, remove unnecesary if

d3c41d0

Signed-off-by: mabhi <[email protected]>

mabhi force-pushed the remove-encoooxor branch from b43f037 to d3c41d0 Compare March 15, 2023 13:01

mabhi requested a review from codesome March 16, 2023 04:59

codesome approved these changes Mar 16, 2023

View reviewed changes

tsdb/chunks/head_chunks.go Outdated Show resolved Hide resolved

addressing refactoring

6efa8a9

Signed-off-by: mabhi <[email protected]>

codesome merged commit 8f6d5dc into prometheus:main Mar 16, 2023

		OutOfOrderMask = 0b10000000
		RestoreOutOfOrderMask = 0b01111111

	OutOfOrderMask = 0b10000000
	RestoreOutOfOrderMask = 0b01111111
	OutOfOrderMask = uint8(0b10000000)

	// update the chunk encoding by restoring the original encoding, if needed
	// Extract the encoding from the byte. ChunkDiskMapper uses only the last 7 bits for the encoding.

	func (cdm *ChunkDiskMapper) UnapplyOutOfOrderMask(sourceEncoding chunkenc.Encoding) chunkenc.Encoding {
	func (cdm *ChunkDiskMapper) RemoveMasks(sourceEncoding chunkenc.Encoding) chunkenc.Encoding {

Conversation

mabhi commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

Related Issue:

Proposed Changes

Proof Manifests

Uh oh!

jesusvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mabhi Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mabhi Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mabhi Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mabhi Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesusvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

mabhi commented Mar 15, 2023

Uh oh!

codesome left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mabhi commented Mar 9, 2023 •

edited

Loading

mabhi Mar 13, 2023 •

edited

Loading

mabhi Mar 13, 2023 •

edited

Loading

mabhi Mar 13, 2023 •

edited

Loading

mabhi Mar 13, 2023 •

edited

Loading