From a280b6c2daaa9ad1239a45996fc72eaff502f83b Mon Sep 17 00:00:00 2001
From: Ganesh Vernekar <ganeshvern@gmail.com>
Date: Wed, 6 Oct 2021 15:28:10 +0530
Subject: [PATCH] Fix review comments

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
---
 tsdb/chunkenc/histo.go      | 50 +++++++++++++++++++++++--------------
 tsdb/chunkenc/histo_meta.go | 27 ++++++++++----------
 tsdb/chunkenc/histo_test.go |  2 +-
 3 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/tsdb/chunkenc/histo.go b/tsdb/chunkenc/histo.go
index 972bb8a665..3ab6948d0c 100644
--- a/tsdb/chunkenc/histo.go
+++ b/tsdb/chunkenc/histo.go
@@ -260,41 +260,53 @@ func (a *HistoAppender) Append(int64, float64) {}
 // * the last sample in the chunk was stale while the current sample is not stale
 // It returns an additional boolean set to true if it is not appendable because of a counter reset.
 // If the given sample is stale, it will always return true.
-func (a *HistoAppender) Appendable(h histogram.SparseHistogram) ([]Interjection, []Interjection, bool, bool) {
+func (a *HistoAppender) Appendable(h histogram.SparseHistogram) (posInterjections []Interjection, negInterjections []Interjection, okToAppend bool, counterReset bool) {
 	if value.IsStaleNaN(h.Sum) {
 		// This is a stale sample whose buckets and spans don't matter.
-		return nil, nil, true, false
+		okToAppend = true
+		return
 	}
 	if value.IsStaleNaN(a.sum) {
 		// If the last sample was stale, then we can only accept stale samples in this chunk.
-		return nil, nil, false, false
+		return
+	}
+
+	if h.Count < a.cnt {
+		// There has been a counter reset.
+		counterReset = true
+		return
 	}
 
 	if h.Schema != a.schema || h.ZeroThreshold != a.zeroThreshold {
-		return nil, nil, false, false
+		return
 	}
-	posInterjections, ok := compareSpans(a.posSpans, h.PositiveSpans)
+
+	if h.ZeroCount < a.zcnt {
+		// There has been a counter reset since ZeroThreshold didn't change.
+		counterReset = true
+		return
+	}
+
+	var ok bool
+	posInterjections, ok = compareSpans(a.posSpans, h.PositiveSpans)
 	if !ok {
-		return nil, nil, false, false
+		counterReset = true
+		return
 	}
-	negInterjections, ok := compareSpans(a.negSpans, h.NegativeSpans)
+	negInterjections, ok = compareSpans(a.negSpans, h.NegativeSpans)
 	if !ok {
-		return nil, nil, false, false
+		counterReset = true
+		return
 	}
 
-	if h.Count < a.cnt || h.ZeroCount < a.zcnt {
-		// There has been a counter reset.
-		return nil, nil, false, false
+	if counterResetInAnyBucket(a.posbuckets, h.PositiveBuckets, a.posSpans, h.PositiveSpans) ||
+		counterResetInAnyBucket(a.negbuckets, h.NegativeBuckets, a.negSpans, h.NegativeSpans) {
+		counterReset, posInterjections, negInterjections = true, nil, nil
+		return
 	}
 
-	if counterResetInAnyBucket(a.posbuckets, h.PositiveBuckets, a.posSpans, h.PositiveSpans) {
-		return nil, nil, false, true
-	}
-	if counterResetInAnyBucket(a.negbuckets, h.NegativeBuckets, a.negSpans, h.NegativeSpans) {
-		return nil, nil, false, true
-	}
-
-	return posInterjections, negInterjections, true, false
+	okToAppend = true
+	return
 }
 
 // counterResetInAnyBucket returns true if there was a counter reset for any bucket.
diff --git a/tsdb/chunkenc/histo_meta.go b/tsdb/chunkenc/histo_meta.go
index 47d7a78b70..9308993ce5 100644
--- a/tsdb/chunkenc/histo_meta.go
+++ b/tsdb/chunkenc/histo_meta.go
@@ -42,36 +42,37 @@ func putHistoChunkMetaSpans(b *bstream, spans []histogram.Span) {
 	}
 }
 
-func readHistoChunkMeta(b *bstreamReader) (bool, int32, float64, []histogram.Span, []histogram.Span, error) {
-	header, err := b.ReadByte()
+func readHistoChunkMeta(b *bstreamReader) (counterReset bool, schema int32, zeroThreshold float64, posSpans []histogram.Span, negSpans []histogram.Span, err error) {
+	var header byte
+	header, err = b.ReadByte()
 	if err != nil {
-		return false, 0, 0, nil, nil, err
+		return
 	}
 
-	counterReset := (header & counterResetMask) != 0
+	counterReset = (header & counterResetMask) != 0
 
 	v, err := readInt64VBBucket(b)
 	if err != nil {
-		return false, 0, 0, nil, nil, err
+		return
 	}
-	schema := int32(v)
+	schema = int32(v)
 
-	zeroThreshold, err := readFloat64VBBucket(b)
+	zeroThreshold, err = readFloat64VBBucket(b)
 	if err != nil {
-		return false, 0, 0, nil, nil, err
+		return
 	}
 
-	posSpans, err := readHistoChunkMetaSpans(b)
+	posSpans, err = readHistoChunkMetaSpans(b)
 	if err != nil {
-		return false, 0, 0, nil, nil, err
+		return
 	}
 
-	negSpans, err := readHistoChunkMetaSpans(b)
+	negSpans, err = readHistoChunkMetaSpans(b)
 	if err != nil {
-		return false, 0, 0, nil, nil, err
+		return
 	}
 
-	return counterReset, schema, zeroThreshold, posSpans, negSpans, nil
+	return
 }
 
 func readHistoChunkMetaSpans(b *bstreamReader) ([]histogram.Span, error) {
diff --git a/tsdb/chunkenc/histo_test.go b/tsdb/chunkenc/histo_test.go
index 71ca36baaa..cb5a3734eb 100644
--- a/tsdb/chunkenc/histo_test.go
+++ b/tsdb/chunkenc/histo_test.go
@@ -258,7 +258,7 @@ func TestHistoChunkAppendable(t *testing.T) {
 		require.Equal(t, 0, len(posInterjections))
 		require.Equal(t, 0, len(negInterjections))
 		require.False(t, ok) // Need to cut a new chunk.
-		require.False(t, cr)
+		require.True(t, cr)
 	}
 
 	{ // New histogram that has a counter reset while buckets are same.