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

free · 2018-05-09T15:27:33Z

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

closes #328

Signed-off-by: Alin Sinpalean [email protected]

Signed-off-by: Alin Sinpalean <[email protected]>

free · 2018-05-14T09:21:25Z

Ping?

gouthamve

Sorry I took longer to review, was busy with a couple of things. LGTM, except for minor nits.

Thanks for this!

gouthamve · 2018-05-16T11:46:42Z

querier.go

+	iCur := it.i
+	for ; it.i < len(it.chunks); it.i++ {
+		// Skip chunks with MaxTime < t.
+		if it.chunks[it.i].MaxTime >= t {


This can be simplified to:

if it.chunks[it.i].MaxTime >= t { continue } // Don't reset the iterator unless we've moved on to a different chunk. if it.i != iCur { it.cur = it.chunks[it.i].Chunk.Iterator() if len(it.intervals) > 0 { it.cur = &deletedIterator{it: it.cur, intervals: it.intervals} } } for it.cur.Next() { t0, _ := it.cur.At() if t0 > it.maxt || it.cur.Err() != nil { return false } if t0 >= t { return true } }

This reduces the indentation and makes it easier to read.

Done, thanks.

gouthamve · 2018-05-16T11:51:13Z

querier_test.go

@@ -987,7 +1026,7 @@ func TestSeriesIterator(t *testing.T) {
 						{10, 22}, {203, 3493},
 					},

-					seek:    203,
+					seek:    101,


Why would this return false?

Because the [mint, maxt] range is [2, 202] and there are no points in the [101, 202] range.

Previously the test was doing a seek() past maxt, which returned immediately because there is an explicit check for that. With the earlier code seek(101) was returning true and positioning the iterator at time 203, which was incorrect.

Signed-off-by: Alin Sinpalean <[email protected]>

free

All done, PTAL.

free · 2018-05-16T13:11:54Z

querier.go

+	iCur := it.i
+	for ; it.i < len(it.chunks); it.i++ {
+		// Skip chunks with MaxTime < t.
+		if it.chunks[it.i].MaxTime >= t {


Done, thanks.

free · 2018-05-16T13:15:47Z

querier_test.go

@@ -987,7 +1026,7 @@ func TestSeriesIterator(t *testing.T) {
 						{10, 22}, {203, 3493},
 					},

-					seek:    203,
+					seek:    101,


Because the [mint, maxt] range is [2, 202] and there are no points in the [101, 202] range.

Previously the test was doing a seek() past maxt, which returned immediately because there is an explicit check for that. With the earlier code seek(101) was returning true and positioning the iterator at time 203, which was incorrect.

free · 2018-05-24T09:09:34Z

Ping?

fabxc · 2018-05-28T19:26:12Z

@gouthamve ^ can you take a look at this?

fabxc · 2018-05-28T20:50:09Z

@free thanks for all the analysis in the issue – seems we've missed quite a few things.
In (1) you mention a particular test that even just adapts the expected result to pass. If this PR fixes (1) though, why is the test not fixed but still passes?

free · 2018-06-05T12:44:40Z

[Apologies for dropping this, GitHub (or some mail server) decided I'm probably not interested in email notifications from this PR.]

In (1) you mention a particular test that even just adapts the expected result to pass. If this PR fixes (1) though, why is the test not fixed but still passes?

Seems like that particular test was actually only confusing rather than actually broken: the test code at line 1066 actually filters out samples that fall outside [mint, maxt] from the expected output (when testing a chunked iterator), but not below (when testing a chained iterator, which is supposed to return the t = 3 value).

I'd rather not go down the rabbit hole of rewriting the unit tests, though. I do have a day job that doesn't involve as much Prometheus as I might otherwise like.

alin-amana · 2018-06-11T08:52:51Z

@gouthamve @fabxc: Ping?

free · 2018-06-11T09:01:11Z

querier.go

-			return false
-		}
+	// Return early if already past t.
+	if t0, _ := it.cur.At(); t0 >= t {


Just realized that this might call At() on an iterator that Next() or Seek() might not have been called on. It works if one assumes all timestamps to be strictly positive and that the iterator will return a timestamp of 0 when uninitialized.

Should I remove it?

Well, crap. This is the thing that makes Seek() not advance the iterator when seeking for a timestamp that's in the past (compared to the iterator's position).

So it looks to me like we need to change the contract of either chunkenc.Iterator.At() to return something like math.MinInt64; or that of tsdb.SeriesIterator.Seek() to always advance the iterator. The latter seems more radical.

I've changed chunkenc.Iterator.At() to always return math.MinInt64 before it has advanced (and documented it as such). Everything is good in the world again.

Yep, before the contract used to be that At() shouldn't be called before Next() or Seek() and it shouldn't be called after they return false.

I like the newer contract of returning math.MinInt64 for non-initialised Iterators.

Signed-off-by: Alin Sinpalean <[email protected]>

krasi-georgiev · 2018-06-11T12:53:22Z

querier_test.go

@@ -1116,6 +1155,24 @@ func TestChunkSeriesIterator_DoubleSeek(t *testing.T) {
 	testutil.Equals(t, float64(2), v)
 }

+// Regression for: https://github.com/prometheus/tsdb/pull/327


I think the test itself explains the desired behaviour so don't think we even need to link it to the issue.

/pull/327 was closed so If you prefer to keep it there maybe point this to https://github.com/prometheus/tsdb/issues/328

krasi-georgiev · 2018-06-11T13:04:24Z

querier.go

@@ -783,6 +783,9 @@ func newChunkSeriesIterator(cs []chunks.Meta, dranges Intervals, mint, maxt int6

 func (it *chunkSeriesIterator) Seek(t int64) (ok bool) {
 	if t > it.maxt {
+		it.i = len(it.chunks)
+	}
+	if it.cur.Err() != nil || it.i >= len(it.chunks) {
 		return false
 	}


so here we are first advancing and than failing. Normally my logic would be that if an operation failed it shouldn't change the internal state.

We're not failing, we're simply advancing to the requested timestamp and returning false, meaning there are no more values.

If we would not advance, then a Seek before maxt following a Seek past maxt would incorrectly position the iterator on a valid value.

Agree with @free and adding exactly same thing to #665

Signed-off-by: Alin Sinpalean <[email protected]>

…anced. Signed-off-by: Alin Sinpalean <[email protected]>

alin-amana · 2018-06-15T08:46:29Z

Any chance of getting this merged?

Although please note that due to the requirement that Seek() not advance the iterator unless necessary, I have had to change the definition of chunkenc.Iterator.At() (and the behavior of its 2 implementations) to return math.MinInt64 before the iterator is advanced.

Newer commits

gouthamve · 2018-07-23T05:40:36Z

querier.go

 		}
 	}
 	return false
 }

 func (it *chunkSeriesIterator) At() (t int64, v float64) {
+	// Should it panic if it.cur.Err() != nil || it.i >= len(it.chunks)?


This comment can be removed.

Right, it is now OK to call this at any time. Done.

gouthamve · 2018-07-23T05:45:02Z

querier.go

-			return false
-		}
+	// Return early if already past t.
+	if t0, _ := it.cur.At(); t0 >= t {


Yep, before the contract used to be that At() shouldn't be called before Next() or Seek() and it shouldn't be called after they return false.

I like the newer contract of returning math.MinInt64 for non-initialised Iterators.

Signed-off-by: Alin Sinpalean <[email protected]>

free · 2018-09-12T10:50:21Z

On Wed, Sep 12, 2018 at 11:47 AM Ganesh Vernekar ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In querier_test.go <#329 (comment)>: > @@ -448,6 +458,18 @@ func TestBlockQuerier(t *testing.T) { ), }), }, + { This test seems to be working even without this PR. Any reason for adding?

[Replying by email because I cant, for the life of me, find this comment on GitHub.] I don't remember specifically, but if I had to guess it's (a) because of a bug I had introduced during my change and wanted to verify whether I had then fixed it; and (b) because it's a use case that's not already covered by the other test cases.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#329 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AelZ0hvWsL9JgPWPBq6ij7CpGZ2zaWS3ks5uaNhCgaJpZM4T4hnj> .

krasi-georgiev · 2019-02-06T15:24:00Z

querier.go

@@ -658,7 +658,7 @@ func (s *chunkSeries) Iterator() SeriesIterator {
 type SeriesIterator interface {
 	// Seek advances the iterator forward to the given timestamp.
 	// If there's no value exactly at t, it advances to the first value
-	// after t.
+	// after t. Has no effect if already past t.


// Seek advances the iterator forward to the given timestamp. // If there's no value exactly at t, it advances to the first value after t. // Seek has no effect if already past t.

something like this reads nicer.

Added to #665 as well

krasi-georgiev · 2019-02-06T15:35:15Z

querier_test.go

+				},
+				chunks: [][]sample{
+					{
+						{1, 2}, {10, 11},


Suggested change

{1, 2}, {10, 11},

{1, 1}, {10, 10},

krasi-georgiev · 2019-02-06T15:44:09Z

querier_test.go

+	}
+
+	// Seek for 3, then 2. Iterator should remain positioned on 3.
+	res := newChunkSeriesIterator(chkMetas, nil, 2, 8)


instead of this can you use the createBlock to create an actual block than just open a db with this block and run the querier and seek against that.
I prefer to use only exported API methods to be as close as possible the expected behaviour for anyone using the library.
the same for TestChunkSeriesIterator_SeekWithMinTime

I know many tests use internal implementation, but I am trying to change that slowly to make these easier to maintain and understand so for any new tests I want to use only exported methods to ensure the correct behaviour.

krasi-georgiev · 2019-02-06T15:54:55Z

chunkenc/chunk.go

@@ -64,6 +65,7 @@ type Appender interface {

 // Iterator is a simple iterator that can only get the next value.
 type Iterator interface {
+	// At returns (math.MinInt64, 0.0) before the iterator has advanced.


should this also be mentioned for the SeriesIterator interface ?

also we need a test to ensure that calling At before Next or Seek returns math.MinInt64

should this also be mentioned for the SeriesIterator interface ?

Done.

also we need a test to ensure that calling At before Next or Seek returns math.MinInt64

Added checks (not separate tests) for all Iterator and SeriesIterator implementations. Let me know if you prefer separate tests or e.g. extracting the check into expandSeriesIterator() instead.

adding this to #665

krasi-georgiev · 2019-02-06T16:21:27Z

querier_test.go

@@ -756,7 +795,7 @@ func TestSeriesIterator(t *testing.T) {
 			b: []tsdbutil.Sample{},
 			c: []tsdbutil.Sample{},

-			seek:    0,
+			seek:    1,


do you remember why you needed to change that?

the test passes even with seek: 0

krasi-georgiev · 2019-02-07T07:54:24Z

querier.go

-		t0, _ := it.cur.At()
-		if t0 >= t {
-			return true
+		// Don't reset the iterator unless we've moved on to a different chunk.


Suggested change

// Don't reset the iterator unless we've moved on to a different chunk.

// Reset the iterator when moved to a different chunk.

I find this a tiny more clear.

adjuste #665 as well, although code looks different (less indents).

krasi-georgiev · 2019-02-07T08:01:30Z

querier_test.go

-			1: Intervals{{1, 3}},
-			2: Intervals{{1, 3}, {6, 10}},
+			1: Intervals{{1, 2}},
+			2: Intervals{{2, 3}, {6, 10}},


Why is this change required?

krasi-georgiev · 2019-03-15T08:10:47Z

ping @free

free · 2019-03-19T16:24:02Z

@krasi-georgiev, as things stand right now, it looks like I have about one more week of heads-down effort on my work project. But I will get back to this as soon as I'm done with 14 hour days. Apologies.

krasi-georgiev · 2019-03-19T17:46:58Z

@free no problem , thanks for letting me know.

krasi-georgiev · 2019-06-18T08:01:43Z

@free is this ready for another review? Not sure if the comments I left are still relevant after the latest changes.

free

@free is this ready for another review? Not sure if the comments I left are still relevant after the latest changes.

Nope, not yet, I've only addressed some of the comments. And, as it turns out, I haven't pushed my changes yet.

But since this will submit all my comments (without the underlying code changes, which are on my home computer) I might as well ask what's your preference for "a test to ensure that calling At before Next or Seek returns math.MinInt64": currently I've added explicit checks wherever an Iterator or SeriesIterator is created, just before it is unrolled by expandSeriesIterator(). Would you prefer for the test to be within expandSeriesIterator() (and thus for a *testing.T parameter to be added to it)? Or just for single, separate tests for every iterator implementation?

free · 2019-06-16T20:33:13Z

chunkenc/chunk.go

@@ -64,6 +65,7 @@ type Appender interface {

 // Iterator is a simple iterator that can only get the next value.
 type Iterator interface {
+	// At returns (math.MinInt64, 0.0) before the iterator has advanced.


should this also be mentioned for the SeriesIterator interface ?

Done.

also we need a test to ensure that calling At before Next or Seek returns math.MinInt64

Added checks (not separate tests) for all Iterator and SeriesIterator implementations. Let me know if you prefer separate tests or e.g. extracting the check into expandSeriesIterator() instead.

free · 2019-06-16T20:36:27Z

querier.go

-		t0, _ := it.cur.At()
-		if t0 >= t {
-			return true
+		// Don't reset the iterator unless we've moved on to a different chunk.


free · 2019-06-16T20:38:06Z

querier.go

@@ -658,7 +658,7 @@ func (s *chunkSeries) Iterator() SeriesIterator {
 type SeriesIterator interface {
 	// Seek advances the iterator forward to the given timestamp.
 	// If there's no value exactly at t, it advances to the first value
-	// after t.
+	// after t. Has no effect if already past t.


free · 2019-06-16T20:40:07Z

querier_test.go

+				},
+				chunks: [][]sample{
+					{
+						{1, 2}, {10, 11},


krasi-georgiev · 2019-06-18T11:34:04Z

Nope, not yet,

ping me when ready for a review and I spend some time for a proper review.

bwplotka · 2019-08-13T08:48:39Z

Ok, I added all the improvements from here to #665 so closing this. Also we are merging prometheus/prometheus#5805 so I need to redo my PR anyway as well on Prometheus later on.

cc @krasi-georgiev @free @codesome

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

2c86185

Signed-off-by: Alin Sinpalean <[email protected]>

gouthamve previously requested changes May 16, 2018

View reviewed changes

Address review comments (reduce nesting by continuing early).

d741a3e

Signed-off-by: Alin Sinpalean <[email protected]>

free commented May 16, 2018

View reviewed changes

free commented Jun 11, 2018

View reviewed changes

Have chunkenc.Iterator return math.MinInt64 before having advanced.

af9131c

Signed-off-by: Alin Sinpalean <[email protected]>

krasi-georgiev reviewed Jun 11, 2018

View reviewed changes

free added 2 commits June 12, 2018 16:11

Address review comments.

cb79c8b

Signed-off-by: Alin Sinpalean <[email protected]>

Also have memSafeIterator return math.MinInt64 before iterator is adv…

ca470f0

…anced. Signed-off-by: Alin Sinpalean <[email protected]>

gouthamve reviewed Jul 23, 2018

View reviewed changes

free added 2 commits July 25, 2018 17:42

Merge remote-tracking branch 'upstream/master' into fix_seek

3f68073

Remove out of date comment.

fff4e5f

Signed-off-by: Alin Sinpalean <[email protected]>

Merge remote-tracking branch 'upstream/master' into fix_seek

e5948f7

free force-pushed the fix_seek branch from 3ee616a to e5948f7 Compare February 6, 2019 10:21

krasi-georgiev reviewed Feb 7, 2019

View reviewed changes

free added 2 commits June 16, 2019 11:39

Merge remote-tracking branch 'upstream/master' into fix_seek

e80e75f

Merge branch 'master' into fix_seek

9934b9b

free commented Jun 18, 2019

View reviewed changes

bwplotka mentioned this pull request Aug 9, 2019

Add ChunksIterator method to Series interface. #665

Open

bwplotka closed this Aug 13, 2019

	// Don't reset the iterator unless we've moved on to a different chunk.
	// Reset the iterator when moved to a different chunk.

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

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

Conversation

free commented May 9, 2018 • edited by krasi-georgiev Loading

free commented May 14, 2018

gouthamve left a 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

Choose a reason for hiding this comment

free left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

free commented May 24, 2018

fabxc commented May 28, 2018

fabxc commented May 28, 2018

free commented Jun 5, 2018

alin-amana commented Jun 11, 2018

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 Jun 11, 2018 • 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

alin-amana commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

free commented Sep 12, 2018 via email

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka Aug 13, 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

Choose a reason for hiding this comment

krasi-georgiev commented Mar 15, 2019

free commented Mar 19, 2019

krasi-georgiev commented Mar 19, 2019

krasi-georgiev commented Jun 18, 2019

free left a 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

Choose a reason for hiding this comment

krasi-georgiev commented Jun 18, 2019 • edited Loading

bwplotka commented Aug 13, 2019

free commented May 9, 2018 •

edited by krasi-georgiev

Loading

krasi-georgiev Jun 11, 2018 •

edited

Loading

bwplotka Aug 13, 2019 •

edited

Loading

krasi-georgiev commented Jun 18, 2019 •

edited

Loading