Use pooling for writes to coordinator #942

nikunjgit · 2018-09-25T19:06:00Z

No description provided.

arnikola · 2018-09-25T19:11:15Z

src/query/pools/query_pools.go

+	}
+
+	writeWorkerPool := xsync.NewWorkerPool(writePoolSize)
+	return objectPool, writeWorkerPool, instrumentOptions


We're getting quite a few things returns here, consider bundling it? I foresee a lot more pools being added as we work on perf :P

Wanna use the new worker pool I wrote for the m3db ingesters? Its way more performant because it doesn't allocate goroutines all the time: https://github.com/m3db/m3x/blob/master/sync/pooled_worker_pool.go

That should help fix a lot of the runtime.morestack/runtime.growstack/runtime.copystack in the m3coordinator flame graphs

I'm pretty sure its API compatible too so I think you can literally just replace the constructors and be done

arnikola · 2018-09-25T19:23:57Z

src/query/storage/m3/storage.go

-	}
+	tagIterator := storage.TagsToIdentTagIterator(query.Tags)
+
+	var (


Considering how this is being broken up, it may be a good idea to use the pooled workerpools like read does? That way every incoming request will get a set of workers for all datapoints in it and we don't have situations where some requests get starved

If we want to simplify it a bit, can revisit my m3x PR at: m3db/m3x#181 and make it a first class citizen, then refactor the readerpools too?

Alternatively, just use the same pool as the readerpools here if you agree with taking this approach

Can you explain the rationale for pooled worker pools? Kind of confused by that

General idea was to give each incoming request its own pool while having a static max size, so that the more expensive requests would not tie up smaller requests, if that's not the correct approach, happy to drop it and revert to your pooled worker pool

Sync'd up in person and settled on (pending discussion with @nikunjgit) replacing the existing WorkerPool with PooledWorkerPool and then getting rid of the generic ObjectPool on top of that

i'll use a PooledWorkerPool for writes and let @arnikola merge the read object pool in another diff. Sound good ?

arnikola · 2018-09-25T19:29:29Z

src/query/storage/m3/storage.go

+	)
+
+	for _, datapoint := range query.Datapoints {
+		s.writeWorkerPool.Go(func() {


I think you need to capture the datapoint variable here; surprised that there are no failing tests... might be good to add one that tries to write multiple datapoints and checks that the correct ones are being written?

good point.

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

src/query/storage/m3/storage.go

+	for _, datapoint := range query.Datapoints {
+		s.writeWorkerPool.Go(func() {
+			wg.Add(1)
+			tagIter := tagIterator.Duplicate()


Are these threadsafe? Also since these all use the same iterator, can you define this outside of the loop?

yeah makes sense

arnikola · 2018-09-25T19:33:15Z

src/query/storage/m3/storage.go

@@ -257,20 +260,28 @@ func (s *m3storage) Write(
 	id := query.Tags.ID()


Maybe we should consider renaming WriteQuery to WriteRequest or something, query implies a response to a question

used at a lot of places. I'd prefer to avoid that change in this diff

arnikola · 2018-09-25T19:34:02Z

src/query/storage/m3/storage.go

+	query *storage.WriteQuery,
+	datapoint ts.Datapoint,
+	identID ident.ID,
+	iterator ident.TagIterator) error {


nit:

iterator ident.TagIterator, ) error {

arnikola · 2018-09-25T19:34:56Z

src/query/storage/m3/storage.go

 	var (
 		namespace ClusterNamespace
 		err       error
 	)
-	switch common.attributes.MetricsType {
+	switch query.Attributes.MetricsType {


nit: pull out query.Attributes and s.clusters into variables

i think s.clusters is fine

richardartoul · 2018-09-26T14:50:12Z

src/query/pools/query_pools.go

@@ -53,7 +53,7 @@ func BuildWorkerPools(
 	cfg config.Configuration,
 	logger *zap.Logger,
 	scope tally.Scope,
-) (pool.ObjectPool, instrument.Options) {
+) (pool.ObjectPool, xsync.WorkerPool, instrument.Options) {


Out of curiousity, why is there a pool of worker pools?

Addressed this quesiton in the other comment

richardartoul · 2018-09-26T14:50:53Z

src/query/pools/query_pools.go

+		writePoolSize = defaultWorkerPoolSize
+	}
+
+	writeWorkerPool := xsync.NewWorkerPool(writePoolSize)


I think you need to call init here

BertHartm · 2018-09-27T15:07:11Z

I'm happy to test this under load if it's at a point where that would make sense.

matejzero · 2018-09-27T15:55:18Z

I'm happy to test this under load if it's at a point where that would make sense.

I'm happy to test too (with around 70k metrics/s).

Duplicate iterator Comments

codecov · 2018-09-28T01:38:55Z

Codecov Report

Merging #942 into master will decrease coverage by <.01%.
The diff coverage is 72.34%.

@@            Coverage Diff             @@
##           master     #942      +/-   ##
==========================================
- Coverage   77.88%   77.88%   -0.01%     
==========================================
  Files         410      410              
  Lines       34363    34373      +10     
==========================================
+ Hits        26765    26770       +5     
- Misses       5750     5755       +5     
  Partials     1848     1848

Flag	Coverage Δ
#dbnode	`81.46% <ø> (+0.03%)`	⬆️
#m3ninx	`75.25% <ø> (-0.08%)`	⬇️
#query	`64.23% <72.34%> (-0.13%)`	⬇️
#x	`80.55% <ø> (ø)`	⬆️

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 e483028...db4f6d4. Read the comment docs.

arnikola · 2018-09-28T16:21:47Z

src/query/test/m3/storage.go

@@ -54,6 +55,9 @@ func NewStorageAndSession(
 		Retention:   TestRetention,
 	})
 	require.NoError(t, err)
-	storage := m3.NewStorage(clusters, nil)
+	writePool, err := sync.NewPooledWorkerPool(10, sync.NewPooledWorkerPoolOptions())


Consider a test to make sure we're using the pools?

arnikola · 2018-09-28T16:22:17Z

src/query/storage/m3/storage.go

-		w.timestamp, w.value, common.unit, common.annotation)
-}
-
-type writeRequestCommon struct {


arnikola reviewed Sep 25, 2018

View reviewed changes

richardartoul reviewed Sep 26, 2018

View reviewed changes

arnikola mentioned this pull request Sep 27, 2018

Set namespace and ID as NoFinalize #956

Merged

Use pooling for writes

d350d1a

Duplicate iterator Comments

nikunjgit force-pushed the nikunj/writeWorkerpool branch from beb200b to d350d1a Compare September 28, 2018 01:38

nikunjgit added 2 commits September 27, 2018 22:09

Fix test

d9344bb

Fix lint

db4f6d4

arnikola approved these changes Sep 28, 2018

View reviewed changes

nikunjgit merged commit 2ec02cb into master Sep 28, 2018

nikunjgit deleted the nikunj/writeWorkerpool branch September 28, 2018 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pooling for writes to coordinator #942

Use pooling for writes to coordinator #942

nikunjgit commented Sep 25, 2018

arnikola Sep 25, 2018

richardartoul Sep 26, 2018

richardartoul Sep 26, 2018

arnikola Sep 25, 2018

richardartoul Sep 26, 2018

arnikola Sep 26, 2018

richardartoul Sep 26, 2018

nikunjgit Sep 28, 2018

arnikola Sep 28, 2018

arnikola Sep 25, 2018

nikunjgit Sep 28, 2018

arnikola Sep 25, 2018

nikunjgit Sep 28, 2018

arnikola Sep 25, 2018

nikunjgit Sep 28, 2018

arnikola Sep 25, 2018

arnikola Sep 25, 2018

nikunjgit Sep 28, 2018

richardartoul Sep 26, 2018

arnikola Sep 26, 2018

richardartoul Sep 26, 2018

nikunjgit Sep 28, 2018

BertHartm commented Sep 27, 2018

matejzero commented Sep 27, 2018

codecov bot commented Sep 28, 2018 •

edited

Loading

arnikola Sep 28, 2018

arnikola Sep 28, 2018

		@@ -257,20 +260,28 @@ func (s *m3storage) Write(
		id := query.Tags.ID()

Use pooling for writes to coordinator #942

Use pooling for writes to coordinator #942

Conversation

nikunjgit commented Sep 25, 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

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

BertHartm commented Sep 27, 2018

matejzero commented Sep 27, 2018

codecov bot commented Sep 28, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 28, 2018 •

edited

Loading