Add ChunksIterator method to Series interface. #665

bwplotka · 2019-07-20T19:15:09Z

This is alternative to the #659 I feel like iterators is the way to go here as we want to iterate & stream chunks instead of buffering all.

Both PRs are for same use case: Allowing Querier to return raw chunks if asked. This will help massively on prometheus/prometheus#5703

This iterators implementation is easy here but will be more important & tricky for head in Prometheus.

fixes https://github.com/prometheus/tsdb/issues/328

Signed-off-by: Bartek Plotka bwplotka@gmail.com

querier.go

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

bwplotka · 2019-07-29T17:26:18Z

Addressed comments @codesome PTAL

querier.go

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

… method. Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

…o far. TODO: tests & splitting into 2. Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

bwplotka · 2019-08-01T17:08:37Z

Added bit yolo implementation of verticalMergeChunkIterator - need to finish edge cases and add tests, but let me know your thoughts @brian-brazil @codesome

codesome

The approach looks fine to me.

querier.go

codesome · 2019-08-02T09:12:05Z

querier.go

+}
+
+func (it *noSeekSeriesIterator) Seek(t int64) bool {
+	it.err = errors.New("not implemented: Seek method invoked for noSeekSeriesIterator")


I would rather prefer having an inefficient implementation as @brian-brazil said instead of an error here.

querier.go

TODO: Add tests. Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

bwplotka · 2019-08-02T13:42:33Z

@brian-brazil very true. But we need to split chunks from time to time as mentioned here: https://github.com/prometheus/tsdb/issues/670 so I would reencode in place here.

Changed the code to reuse more stuff around, let me know how that looks.

chunkenc/chunk.go

querier.go

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

krasi-georgiev · 2019-08-06T08:16:31Z

CHANGELOG.md

 ## 0.10.0

 - [FEATURE] Added `DBReadOnly` to allow opening a database in read only mode.
    - `DBReadOnly.Blocks()` exposes a slice of `BlockReader`s.
    - `BlockReader` interface - removed MinTime/MaxTime methods and now exposes the full block meta via `Meta()`.
 - [FEATURE] `chunckenc.Chunk.Iterator` method now takes a `chunckenc.Iterator` interface as an argument for reuse.
+ - [CHANGE] `Series` interface allows return chunk iterator that allows iterating over encoded chunks.


wrong paragraph?

failed rebase

krasi-georgiev · 2019-08-07T11:55:12Z

querier.go

+		return false
+	}
+
+	for it.Next() {


I think when the current time is the requested time calling Seek should not advance the iterator.

Suggested change

for it.Next() {

if atT, _ := it.At(); t >= atT {

return true

}

for it.Next() {

the same for all places.

I am basing this behavior basd on @gouthamve's comment in https://github.com/prometheus/tsdb/issues/328#issuecomment-387739347

Should also add this to ensure the behavior in the tests.

Awesome! I will add decisions there to Seek interface comment and add tests for this.

querier.go

krasi-georgiev · 2019-08-07T20:05:59Z

querier.go

 	return newVerticalMergeSeriesIterator(s.series...)
 }

-// verticalMergeSeriesIterator implements a series iterater over a list
+func (s *verticalChainedSeries) ChunkIterator() ChunkIterator {


I think it would be useful to also add a test for this.
It will help visualize what output is expected with a given input.

Is there a test that covers this?

krasi-georgiev · 2019-08-07T20:29:28Z

querier.go

+	}
+	return &verticalMergeChunkIterator{
+		a: s[0].ChunkIterator(),
+		b: newVerticalMergeChunkIterator(s[1:]...),


I find this really hard to understand. I know it is used in the verticalMergeSeriesIterator as well, but will be really happy if we simplify this at some point.
Not a blocker for this PR thought. I just thought I would mention it 😈

The recursive approach is sometimes hard to follow indeed. Let's postpone cleaning this though, as we do this everywhere.

I did run a debug session and the final struct is really bizarre, sort of deeply nested.

But yeah a conversation for another PR.

bwplotka · 2019-08-08T12:53:22Z

Thanks @krasi-georgiev for review. Let me add few unit tests to cover all iterators I added (:

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

bwplotka · 2019-08-08T15:27:34Z

querier.go

-	// Err returns the current error.
-	Err() error
+func (s *chunkSeries) ChunkIterator() ChunkIterator {
+	return &chunkIterator{chunks: s.chunks}


newChunkSeriesIterator accepts mint and maxt. Should we do the same here? @brian-brazil @krasi-georgiev ? I think we should.

However not sure what mint maxt means here in detail. Should we be strict and allow mint and maxt in terms of samples inside chunks as well?

By the looks of this: https://github.com/prometheus/tsdb/blob/78e9e360581153d73b1c69d31fd6e2d89e060a82/querier.go#L796 we just check if they overlap with required min or maxt here.

Then, we pass to newChunkSeriesIterator with mint maxt so I am pretty sure we need to be strict here as well in ChunkIterator. E.g when we query block [0, 20)with querier restricted to[10, 40]we need to return chunk[10,19]even if the block has[0, 19]on thisChunkIterator` level, I assume.

newChunkSeriesIterator accepts mint and maxt. Should we do the same here? @brian-brazil @krasi-georgiev ? I think we should.

I think it is fine to start with with the current interface if it covers the streaming use case. Don't see a problem to refactor when/if needed.

* Simplified; merged seek and non seek cases together. Added explicit min/max only for chunk series iterator, where it is relevant. * Adjusted all seek implementation to match edge case requirement (double seek, failed seek + next). Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

bwplotka · 2019-08-08T18:27:41Z

querier.go

@@ -60,6 +60,19 @@ type Series interface {
 	ChunkIterator() ChunkIterator
 }

+// ChunkIterator iterates over the chunk of a time series.
+type ChunkIterator interface {


Moved to be near Series interface.

bwplotka · 2019-08-08T18:29:15Z

querier_test.go

 	// Do binary search between current position and end.
-	it.idx = sort.Search(len(it.list)-it.idx, func(i int) bool {


Problem here was that sort.Search on not found gives the index to put the value in. This makes sense but seems like we forgot that we operate on shorter len(it.list)-it.idx list (:

bwplotka · 2019-08-09T09:27:20Z

With new changes this PR also fixes https://github.com/prometheus/tsdb/issues/328

krasi-georgiev

Does this cover all changes from #329 so I can close it in favor of this one?

CHANGELOG.md

chunkenc/chunk.go

chunkenc/chunk_test.go

querier.go

querier_test.go

krasi-georgiev · 2019-08-09T13:29:43Z

querier.go

-	// Err returns the current error.
-	Err() error
+func (s *chunkSeries) ChunkIterator() ChunkIterator {
+	return &chunkIterator{chunks: s.chunks}


newChunkSeriesIterator accepts mint and maxt. Should we do the same here? @brian-brazil @krasi-georgiev ? I think we should.

I think it is fine to start with with the current interface if it covers the streaming use case. Don't see a problem to refactor when/if needed.

krasi-georgiev · 2019-08-09T13:30:42Z

querier.go

 	return newVerticalMergeSeriesIterator(s.series...)
 }

-// verticalMergeSeriesIterator implements a series iterater over a list
+func (s *verticalChainedSeries) ChunkIterator() ChunkIterator {


Is there a test that covers this?

krasi-georgiev · 2019-08-09T13:32:32Z

querier.go

+	}
+	return &verticalMergeChunkIterator{
+		a: s[0].ChunkIterator(),
+		b: newVerticalMergeChunkIterator(s[1:]...),


I did run a debug session and the final struct is really bizarre, sort of deeply nested.

But yeah a conversation for another PR.

bwplotka · 2019-08-09T13:48:40Z

@krasi-georgiev

I am adding tests for ChunkIterator now
I did not know about Fix issues (1), (2) and (3) described in #328 #329 😱 I will sync on the discussions there and ensure I addressed all comments from there, then we can close if @free is ok with that.

cc @free if you are happy with that. I am touching this place as well.

brian-brazil · 2019-08-12T11:22:48Z

querier.go

+	// Seek advances the iterator forward to the given timestamp.
+	// It advances to the chunk with min time at t or first chunk with min time after t.
+	Seek(t int64) bool
+	// At returns the meta.


"current value". This makes it sounds like it's dropping memes :)

chunkenc/chunk.go

bwplotka requested review from tomwilkie, gouthamve, krasi-georgiev and codesome July 20, 2019 19:15

bwplotka mentioned this pull request Jul 20, 2019

[WIP] Add Chunks method to Series interface. #659

Closed

bwplotka force-pushed the add-chunks-to-queriers-iterator branch from 7feb231 to 291ab8e Compare July 20, 2019 20:54

codesome reviewed Jul 26, 2019

View reviewed changes

querier.go Show resolved Hide resolved

querier.go Outdated Show resolved Hide resolved

querier.go Show resolved Hide resolved

querier.go Outdated Show resolved Hide resolved

Add ChunksIterator method to Series interface.

64eeed9

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

bwplotka force-pushed the add-chunks-to-queriers-iterator branch from 291ab8e to 64eeed9 Compare July 29, 2019 17:26

brian-brazil reviewed Jul 30, 2019

View reviewed changes

querier.go Show resolved Hide resolved

bwplotka added 3 commits July 31, 2019 12:23

Added commentary.

4ed00e1

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

Return not implemented error on vertical iterators for not used chunk…

d097d3f

… method. Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

Added simple verticalMergeChunkIterator implementation - no tests s…

c407499

…o far. TODO: tests & splitting into 2. Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

codesome reviewed Aug 2, 2019

View reviewed changes

bwplotka added 2 commits August 2, 2019 13:42

Added Seek to Chunk/ChunkIterator interfaces.

af8fb41

TODO: Add tests. Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

Reusing same vertical merging strategy in both compact and querier.

6f6dd39

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

brian-brazil reviewed Aug 5, 2019

View reviewed changes

chunkenc/chunk.go Outdated Show resolved Hide resolved

querier.go Outdated Show resolved Hide resolved

Reused same interface in chunkenc.

78e9e36

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

krasi-georgiev reviewed Aug 7, 2019

View reviewed changes

bwplotka added 2 commits August 8, 2019 13:53

Fixed CHANGELOG.

f09fb89

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

Added tests to xor iterator seek.

220c3dd

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

bwplotka commented Aug 8, 2019

View reviewed changes

bwplotka force-pushed the add-chunks-to-queriers-iterator branch from d60075a to b84c439 Compare August 8, 2019 18:27

bwplotka commented Aug 8, 2019

View reviewed changes

krasi-georgiev reviewed Aug 9, 2019

View reviewed changes

brian-brazil reviewed Aug 12, 2019

View reviewed changes

This was referenced Aug 13, 2019

Fix issues (1), (2) and (3) described in #328 #329

Closed

Add ChunksIterator method to Series interface. prometheus/prometheus#5882

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ChunksIterator method to Series interface. #665

Add ChunksIterator method to Series interface. #665

bwplotka commented Jul 20, 2019 •

edited by krasi-georgiev

Loading

bwplotka commented Jul 29, 2019

bwplotka commented Aug 1, 2019

codesome left a comment

codesome Aug 2, 2019

bwplotka commented Aug 2, 2019

krasi-georgiev Aug 6, 2019

bwplotka Aug 8, 2019

krasi-georgiev Aug 7, 2019

bwplotka Aug 8, 2019

krasi-georgiev Aug 7, 2019 •

edited

Loading

krasi-georgiev Aug 9, 2019

krasi-georgiev Aug 7, 2019

bwplotka Aug 8, 2019

krasi-georgiev Aug 9, 2019

bwplotka commented Aug 8, 2019

bwplotka Aug 8, 2019

bwplotka Aug 8, 2019 •

edited

Loading

krasi-georgiev Aug 9, 2019

bwplotka Aug 8, 2019

bwplotka Aug 8, 2019

bwplotka commented Aug 9, 2019

krasi-georgiev left a comment •

edited

Loading

krasi-georgiev Aug 9, 2019

krasi-georgiev Aug 9, 2019

krasi-georgiev Aug 9, 2019

bwplotka commented Aug 9, 2019 •

edited

Loading

brian-brazil Aug 12, 2019

		// Do binary search between current position and end.
		it.idx = sort.Search(len(it.list)-it.idx, func(i int) bool {

Add ChunksIterator method to Series interface. #665

Are you sure you want to change the base?

Add ChunksIterator method to Series interface. #665

Conversation

bwplotka commented Jul 20, 2019 • edited by krasi-georgiev Loading

bwplotka commented Jul 29, 2019

bwplotka commented Aug 1, 2019

codesome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Aug 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krasi-georgiev Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Aug 8, 2019

Choose a reason for hiding this comment

bwplotka Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Aug 9, 2019

krasi-georgiev left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Aug 9, 2019 • edited Loading

Choose a reason for hiding this comment

bwplotka commented Jul 20, 2019 •

edited by krasi-georgiev

Loading

krasi-georgiev Aug 7, 2019 •

edited

Loading

bwplotka Aug 8, 2019 •

edited

Loading

krasi-georgiev left a comment •

edited

Loading

bwplotka commented Aug 9, 2019 •

edited

Loading