Add ability for query storage to provide unconsolidated blocks #929

nikunjgit · 2018-09-21T18:36:49Z

No description provided.

benraskin92 · 2018-09-21T19:09:02Z

src/query/block/column.go

@@ -36,6 +36,11 @@ type columnBlock struct {
 	seriesMeta []SeriesMeta
 }

+// Unconsolidated returns the unconlidated version for the block


s/unconlidated/unconsolidated

benraskin92 · 2018-09-21T19:09:16Z

src/query/block/column.go

@@ -36,6 +36,11 @@ type columnBlock struct {
 	seriesMeta []SeriesMeta
 }

+// Unconsolidated returns the unconlidated version for the block
+func (c *columnBlock) Unconsolidated() (UnconsolidatedBlock, error) {
+	return nil, fmt.Errorf("unconslidated block not support for block, meta: %s", c.meta)


Reword error and check spelling

benraskin92 · 2018-09-21T19:09:53Z

src/query/block/scalar.go

@@ -47,6 +47,11 @@ func NewScalar(val float64, bounds models.Bounds) Block {
 	}
 }

+// Unconsolidated returns the unconlidated version for the block


Same comments as above

benraskin92 · 2018-09-21T19:13:38Z

src/query/block/series.go

+	return len(s.datapoints)
+}
+
+// Consolidated conslidates the series


s/conslidates/consolidates

benraskin92 · 2018-09-21T19:19:52Z

src/query/block/types.go

+}
+
+// Type assertion
+var _ ConsolidationFunc = TakeLast


What is this needed for exactly?

to ensure TakeLast actually implements ConsolidationFunc. I guess i can remove it since multiSeriesBlock Consolidate will throw a compilation error if TakeLast doesn't implement it

benraskin92 · 2018-09-21T19:20:57Z

src/query/executor/transform/lazy.go

@@ -145,6 +145,23 @@ type lazyBlock struct {
 	processedBlock block.Block
 }

+// Unconsolidated returns the unconlidated version for the block


s/unconlidated/unconsolidated

s/for the block/of the block

we use "for the block" in most other places.

benraskin92 · 2018-09-21T19:33:34Z

src/query/executor/transform/lazy.go

+		return f.processedBlock.Unconsolidated()
+	}
+
+	err := f.process()


if err := f.process(); err != nil { return nil, err }

benraskin92 · 2018-09-21T20:10:26Z

src/query/functions/temporal/base_test.go

 	sum := 0.0
-	for _, n := range f {
+	for _, n := range vals {


benraskin92 · 2018-09-21T20:19:36Z

src/query/storage/consolidate.go

+}
+
+func (c *consolidatedBlock) Unconsolidated() (block.UnconsolidatedBlock, error) {
+	return nil, fmt.Errorf("unconsolidated blocks are not supported")


errors.New("unconsolidated blocks are not supported")

benraskin92 · 2018-09-21T20:40:15Z

src/query/storage/consolidate.go

+	return c.unconsolidated.Close()
+}
+
+type consolidateStepIter struct {


consolidatedStepIter?

benraskin92 · 2018-09-21T20:42:41Z

src/query/storage/consolidate.go

+	return c.unconsolidated.Meta()
+}
+
+type consolidateSeriesIter struct {


consolidatedSeriesIter?

arnikola · 2018-09-21T19:26:01Z

src/query/block/scalar.go

@@ -47,6 +47,11 @@ func NewScalar(val float64, bounds models.Bounds) Block {
 	}
 }

+// Unconsolidated returns the unconlidated version for the block
+func (b *Scalar) Unconsolidated() (UnconsolidatedBlock, error) {
+	return nil, fmt.Errorf("unconslidated block not support for block, meta: %s", b.meta)


This should probably be a notImplemented error, since scalars should be fine to have unconsolidated

arnikola · 2018-09-21T19:27:10Z

src/query/block/series.go

+	return UnconsolidatedSeries{datapoints: datapoints, Meta: meta}
+}
+
+// DatapointsAtStep returns the raw datapoints at a step index


nit: DatapointAtStep returns the raw datapoint at a step index

it's returning multiple datapoints ?

arnikola · 2018-09-21T19:27:32Z

src/query/block/series.go

+
+// DatapointsAtStep returns the raw datapoints at a step index
+func (s UnconsolidatedSeries) DatapointsAtStep(idx int) ts.Datapoints {
+	return s.datapoints[idx]


Should perform bound checks

arnikola · 2018-09-21T19:31:33Z

src/query/block/types.go

 )

 // Block represents a group of series across a time bound
 type Block interface {
+	// Unconsolidated returns the unconsolidated version of the block
+	Unconsolidated() (UnconsolidatedBlock, error)


Maybe rather than error it could return a bool to indicate if it can be unconsolidated?

Even for things which can support unconsolidated, you can still get an error. So we need an error here neverthless. I like this over having a bool so that clients don't check 2 things

A bit weird; you may want to continue and do something different if you can't consolidate because you errored vs your block being unable to block, but that could be changed later if necessary

arnikola · 2018-09-21T19:56:39Z

src/query/test/block.go

+func seriesValuesToDatapoints(values []float64, bounds models.Bounds) ts.Datapoints {
+	dps := make(ts.Datapoints, len(values))
+	for i, v := range values {
+		t, _ := bounds.TimeForIndex(i)


I think you need the second arg here otherwise you'll have a nil timestamp

I was assuming we'd just provide values within bounds for tests. Didn't want each test to check for error. A better approach i suppose it for tests to pass in testing.T object but that requires too many changes and didn't look very useful

My bad didn't catch these were test utilities

arnikola · 2018-09-21T20:32:16Z

src/query/block/types.go

+	// SeriesMeta returns the metadata for each series in the block
+	SeriesMeta() []SeriesMeta
+	// Meta returns the metadata for the block
+	Meta() Metadata


Can roll these into a single interface and share that with StepIter

Actually, might be a good idea to pull

// SeriesMeta returns the metadata for each series in the block SeriesMeta() []SeriesMeta // Meta returns the metadata for the block Meta() Metadata

into a single interface since a lot of these use them

arnikola · 2018-09-21T20:41:26Z

src/query/test/block.go

+		dps := seriesValuesToDatapoints(values, bounds)
+		seriesList[i] = ts.NewSeries(meta[i].Name, dps, meta[i].Tags)
+	}
+	b, _ := storage.NewMultiSeriesBlock(seriesList, &storage.FetchQuery{


nit: newline

arnikola · 2018-09-21T20:42:35Z

src/query/storage/consolidate.go

+ * THE SOFTWARE.
+ */
+
+package storage


nit: rename to consolidated.go

arnikola · 2018-09-21T20:42:49Z

src/query/storage/consolidate.go

+	return c.unconsolidated.Close()
+}
+
+type consolidateStepIter struct {


nit: rename to consolidatedStepIter

arnikola · 2018-09-21T20:43:08Z

src/query/storage/consolidate.go

+	return c.unconsolidated.Meta()
+}
+
+type consolidateSeriesIter struct {


nit: rename to consolidatedSeriesIter

benraskin92 · 2018-09-21T20:59:28Z

src/query/test/block.go

+	for i, values := range seriesValues {
+		dps := seriesValuesToDatapoints(values, bounds)
+		seriesList[i] = ts.NewSeries(meta[i].Name, dps, meta[i].Tags)
+	}


nit: newline

benraskin92 · 2018-09-21T21:02:44Z

src/query/ts/values.go

+
+// Aligned returns values aligned to given bounds.
+func (d Datapoints) Aligned(bounds models.Bounds) []Datapoints {
+	numDatapoints := len(d)


benraskin92 · 2018-09-21T21:03:30Z

src/query/ts/values.go

+}
+
+// Aligned returns values aligned to given bounds.
+func (d Datapoints) Aligned(bounds models.Bounds) []Datapoints {


Rename to Align?

@arnikola wanted it AlignToBounds. Is that better ?

Early bird gets the worm @benraskin92 😜

codecov · 2018-09-24T17:31:26Z

Codecov Report

Merging #929 into master will decrease coverage by 21.94%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           master     #929       +/-   ##
===========================================
- Coverage   77.82%   55.88%   -21.95%     
===========================================
  Files         411      408        -3     
  Lines       34516    34377      -139     
===========================================
- Hits        26863    19212     -7651     
- Misses       5778    13374     +7596     
+ Partials     1875     1791       -84

Flag	Coverage Δ
#dbnode	`69.67% <ø> (-11.72%)`	⬇️
#m3ninx	`54.14% <ø> (-21.12%)`	⬇️
#query	`0% <0%> (-64.36%)`	⬇️
#x	`38.88% <ø> (-45.84%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 918ee35...1320a86. Read the comment docs.

benraskin92 · 2018-09-24T18:33:07Z

src/query/block/column.go

@@ -36,6 +36,11 @@ type columnBlock struct {
 	seriesMeta []SeriesMeta
 }

+// Unconsolidated returns the unconsolidated version for the block
+func (c *columnBlock) Unconsolidated() (UnconsolidatedBlock, error) {
+	return nil, fmt.Errorf("unconsolidated view not support for block, meta: %s", c.meta)


s/support/supported

benraskin92 · 2018-09-24T18:45:50Z

src/query/storage/block.go

+func (m multiSeriesBlock) Consolidate() (block.Block, error) {
+	return &consolidatedBlock{
+		unconsolidated:    m,
+		consolidationFunc: block.TakeLast,


We'll use a different consolidation func in the future, right?

prometheus only has gauges so TakeLast is only reasonable option. Yeah, we would probably have different once we have different types.

arnikola · 2018-09-24T20:29:17Z

src/query/block/scalar.go

@@ -47,6 +47,11 @@ func NewScalar(val float64, bounds models.Bounds) Block {
 	}
 }

+// Unconsolidated returns the unconsolidated version for the block


nit ...of the block

Ben had the same comment. We use "for the block" almost everywhere

It's grammatically correct almost everywhere, just not here 😛

arnikola · 2018-09-24T20:32:20Z

src/query/block/types.go

+		return math.NaN()
+	}
+
+	return values[len(values)-1].Value


This might need to be a little more complex; rather than taking the last value it should take the last non-NaN value

arnikola · 2018-09-24T20:37:44Z

src/query/server/server.go

-	select {
-	case <-sigChan:
-	case <-interruptCh:
+	var interruptErr error


Might need to rebase?

arnikola · 2018-09-25T17:52:41Z

src/query/ts/values.go

+func (b *fixedResolutionValues) AlignToBounds(_ models.Bounds) []Datapoints {
+	values := make([]Datapoints, len(b.values))
+	for i := 0; i < b.Len(); i++ {
+		values[i] = Datapoints{b.DatapointAt(i)}


Maybe

stepSize := bound.StepSize() ... Datapoints{ Timestamp: bound.Start().Add(time.Duration(i) * stepSize) Value: b.DatapointAt(i) }

?

This is a little cleaner since Datapoint already does this logic

arnikola · 2018-09-25T18:43:59Z

src/query/block/series.go

+}
+
+// Consolidated consolidates the series
+func (s UnconsolidatedSeries) Consolidated(consolidationFunc ConsolidationFunc) Series {


nit: rename to Consolidate?

it's just talking about a view. Maybe we already have it cached and don't really consolidate.

I like Consolidate

arnikola · 2018-09-25T21:33:14Z

src/query/block/column.go

@@ -36,6 +36,11 @@ type columnBlock struct {
 	seriesMeta []SeriesMeta
 }

+// Unconsolidated returns the unconsolidated version for the block


nit: of the block

we use for the block at most places. Can do a search and replace in a separate diff

arnikola · 2018-09-25T21:38:05Z

src/query/functions/temporal/functions.go

 		return math.NaN()
 	}

 	allNaNs := true
 	result := 0.0
-	prev := values[0]
+	prev := datapoints[0].Value


Any chance these can be nil?

we already have a length check ?

I mean datapoints[0] could be nil, so you might hit NPE when you call .Value

arnikola · 2018-09-25T21:41:58Z

src/query/test/block.go

+func seriesValuesToDatapoints(values []float64, bounds models.Bounds) ts.Datapoints {
+	dps := make(ts.Datapoints, len(values))
+	for i, v := range values {
+		t, _ := bounds.TimeForIndex(i)


My bad didn't catch these were test utilities

arnikola · 2018-09-25T21:43:30Z

src/query/storage/consolidated.go

+	"github.com/m3db/m3/src/query/block"
+)
+
+type consolidatedBlock struct {


nit: consolidatedBlock is a bit of a weird name for something containing an unconsolidated block and a consolidation block

it doesn't have consolidation block just a consolidatationFunc

arnikola · 2018-09-25T21:46:26Z

src/query/ts/values.go

+	dpIdx := 0
+	for i := 0; i < steps; i++ {
+		startIdx := dpIdx
+		t, _ := bounds.TimeForIndex(i)


Then might be a bit better to do the calculation here explicitly to avoid the overhead of the bounds checking in .TimeForIndex()?

arnikola · 2018-09-26T20:18:21Z

src/query/block/series.go

@@ -20,6 +20,10 @@

 package block

+import (
+	"github.com/m3db/m3/src/query/ts"


Can you use "github.com/m3db/m3/src/dbnode/ts" and delete this one?

Ideally we want to avoid any dbnode things especially coupling their series representation with ours. We'd probably move anything m3db related within ts/m3db and localize it there

benraskin92 · 2018-09-28T19:24:32Z

src/query/block/types.go

+// TakeLast is a consolidation function which takes the last datapoint which has non nan value
+func TakeLast(values ts.Datapoints) float64 {
+	for i := len(values) - 1; i >= 0; i-- {
+		if !math.IsNaN(values[i].Value) {


benraskin92 · 2018-09-28T19:56:57Z

src/query/storage/block.go

+		}
+
+		m.consolidated = consolidated
+


nit: newline

benraskin92 · 2018-09-28T20:26:39Z

src/query/ts/values.go

+
+// AlignToBounds returns values aligned to given bounds.
+func (d Datapoints) AlignToBounds(bounds models.Bounds) []Datapoints {
+	numDatapoints := d.Len()


maybe put these in a var()?

var ( numDatapoints = d.Len() steps = bounds.Steps() stepValues = make([]Datapoints, steps) dpIdx = 0 stepSize = bounds.StepSize t = bounds.Start )

benraskin92 · 2018-09-28T20:26:49Z

src/query/ts/values.go

+	dpIdx := 0
+	stepSize := bounds.StepSize
+	t := bounds.Start
+	for i := 0; i < steps; i++ {


nit: newline

benraskin92 · 2018-09-28T20:30:03Z

src/query/ts/values.go

@@ -98,6 +138,17 @@ func (b *fixedResolutionValues) DatapointAt(point int) Datapoint {
 	}
 }

+// AlignToBounds returns values aligned to given bounds.


Kind of weird to call this AlignToBounds, but you just disregard the bounds..

benraskin92

Couple nits, but LGTM

robskillington · 2018-09-30T15:45:37Z

src/query/storage/consolidated.go

+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */


nit: This license text comment should be // prefixed per line like the other .go files.

arnikola

Ran this on a preprod host and the missing metrics we were seeing on master are resolved :)

Fix lint Remove unused mock Revert "Remove unused mock" This reverts commit 2ad6e95. Update mock

nikunjgit requested review from benraskin92 and arnikola September 21, 2018 18:36

benraskin92 reviewed Sep 21, 2018

View reviewed changes

arnikola reviewed Sep 21, 2018

View reviewed changes

benraskin92 reviewed Sep 21, 2018

View reviewed changes

nikunjgit force-pushed the unconsolidatedBlock branch from 4ec7dcb to 7b7972c Compare September 24, 2018 17:31

benraskin92 reviewed Sep 24, 2018

View reviewed changes

prateek force-pushed the master branch from e355dcd to c593af5 Compare September 24, 2018 19:02

nikunjgit force-pushed the unconsolidatedBlock branch 3 times, most recently from b5230be to 2ad6e95 Compare September 25, 2018 17:18

arnikola reviewed Sep 25, 2018

View reviewed changes

arnikola reviewed Sep 26, 2018

View reviewed changes

nikunjgit force-pushed the unconsolidatedBlock branch from b9bc69e to a373c73 Compare September 28, 2018 18:32

benraskin92 reviewed Sep 28, 2018

View reviewed changes

src/query/storage/block.go

}

m.consolidated = consolidated

Copy link

Collaborator

benraskin92 Sep 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: newline

benraskin92 reviewed Sep 28, 2018

View reviewed changes

src/query/ts/values.go

dpIdx := 0

stepSize := bounds.StepSize

t := bounds.Start

for i := 0; i < steps; i++ {

Copy link

Collaborator

benraskin92 Sep 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: newline

benraskin92 reviewed Sep 28, 2018

View reviewed changes

benraskin92 approved these changes Sep 28, 2018

View reviewed changes

robskillington reviewed Sep 30, 2018

View reviewed changes

arnikola approved these changes Oct 1, 2018

View reviewed changes

nikunjgit added 4 commits October 1, 2018 11:38

Fix consolidation

a8415c9

Fix lint Remove unused mock Revert "Remove unused mock" This reverts commit 2ad6e95. Update mock

Comments

1d3da98

Fix cycle

64ac974

Fix license

1320a86

nikunjgit force-pushed the unconsolidatedBlock branch from a373c73 to 1320a86 Compare October 1, 2018 15:39

nikunjgit merged commit 5b31831 into master Oct 1, 2018

nikunjgit deleted the unconsolidatedBlock branch October 1, 2018 16:04

Add ability for query storage to provide unconsolidated blocks #929

Add ability for query storage to provide unconsolidated blocks #929

Conversation

nikunjgit commented Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

benraskin92 Sep 21, 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

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

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

nikunjgit Sep 24, 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

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

codecov bot commented Sep 24, 2018 • edited Loading

Codecov Report

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

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

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

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

benraskin92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola left a comment

Choose a reason for hiding this comment

nikunjgit commented Sep 21, 2018 •

edited

Loading

benraskin92 Sep 21, 2018 •

edited

Loading

nikunjgit Sep 24, 2018 •

edited

Loading

codecov bot commented Sep 24, 2018 •

edited

Loading