pools: option to prefill #4769

sougou · 2019-03-31T16:45:14Z

Background: this change allows one to create prefilled resource
pools. This is useful when traffics suddenly shift while the
pool is still empty. This causes a thundering herd of Open requests
that can cause outage.

There is a proposal to rewrite this pool to natively accommodate
this feature. This change is a stand-in until that effort is
completed.

Signed-off-by: Sugu Sougoumarane ssougou@gmail.com

sougou · 2019-03-31T16:48:30Z

@tirsen @gak Here's the approach that @demmer proposed that can be used as stand-in until we figure out the course of action for #4697. In fact, if this addresses the core problem, we could just look at staying with this until we encounter newer issues in the future.

PR is in draft state because tests need to be written.

tirsen · 2019-03-31T20:55:27Z

@gak 's rewrite tried to address two issues:

Maintaining a minimum amount of connections at all times.
Only allocating the connections necessary.

Correct me if I'm wrong I don't know the connection pool deeply but AFAICT this PR doesn't address any of these issues: It does not address 1) as it will only prefill the pool up front, once connections expire due to idleness the pool is once again empty. It obviously does not address 2) because it will again fill the entire pool before it starts reusing connections.

sougou · 2019-04-01T02:10:12Z

The idle pool closer actually opens a new connection after closing the expired one. So, the pool remains generally full.

I thought of doing the same thing when a closed connection was returned to the pool, but felt that it wasn't worth it. It's rare enough that the overhead should get absorbed.

demmer · 2019-04-25T13:49:38Z

@tirsen to your points:

Maintaining a minimum amount of connections at all times.

I agree with you that it would be cleaner to change this PR so that we never returned a closed connection to the pool. @sougou it seems simple enough to change things so that whenever Put was called with a nil resource and a KeepFull option is enabled, then a new goroutine is spun up to create the connection and then put it back in the pool only when full.

Only allocating the connections necessary.
This is the part of the requirements that seems to motivate a lot of the complexity in the other implementation and which I don't understand.

Can you explain the use case why you would want to allocate N connections up front, allowing a burst up to M, but where you're willing to pay the cost of waiting to connect for the burst spillover?

It really seems to me that if we're worried about connection latency then it'd be better to just preallocate the whole pool's worth.

Background: this change allows one to create prefilled resource pools. This is useful when traffics suddenly shift while the pool is still empty. This causes a thundering herd of Open requests that can cause outage. There is a proposal to rewrite this pool to natively accommodate this feature. This change is a stand-in until that effort is completed. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou · 2019-05-25T14:00:10Z

I've finished the work on this PR. The following actions now happen always, whether you've enabled the prefill feature or not:

The idle closer reopens new connections to replace the ones it closed.
Put opens a new connection if a nil is sent to it (connection was closed).
This means that once a pool is filled, it will remain filled.

The prefill feature is enabled by specifying a non-zero value to a new argument to the resource pool: prefillParallelism. It prefills the pool with open connections and the rate of open is throttled by the specified parallelism. This is exposed through the following options for vttablet:

queryserver-config-pool-prefill-parallelism
queryserver-config-stream-pool-prefill-parallelism
queryserver-config-message-conn-pool-prefill-parallelism
queryserver-config-transaction-prefill-parallelism

sougou · 2019-05-25T14:00:44Z

@demmer @mpawliszyn @tirsen @gak

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

mpawliszyn · 2019-05-28T20:15:56Z

go/pools/resource_pool.go

@@ -170,9 +195,6 @@ func (rp *ResourcePool) get(ctx context.Context, wait bool) (resource Resource,
 	select {
 	case wrapper, ok = <-rp.resources:
 	default:
-		if !wait {


Was this intended? Or is is not used anywhere?

I answered my own question. It was always true.

mpawliszyn

LGTM

demmer · 2019-05-28T20:33:40Z

go/pools/resource_pool.go

+				rp.Put(r)
+			}()
+		}
+		wg.Wait()


Is it always necessary to wait for all the prefill actions to complete here?

I am somewhat reticent to add a ton of optionality here, but especially given the unbounded context.TODO(), this code as written might block startup for an arbitrary amount of time, no?

This is a good point. I'm not sure what the best approach would be. The problem with doing it asynchronously is that we'll get into trouble if that function hangs. Then we'll have to protect it from races with Close, etc. The other option is to add a timeout to the context.

I'm thinking we should let people try this and observe failure modes. That may give us better clarity on the best way forward. I've added log messages in pool.go so we can collect some data about this.

This is the one part of this change that still worries me a bit -- as written this could potentially block startup forever, which seems like a bad idea. It would also be efficient to parallelize this with the other startup tasks.

At the same time if users really want to make sure the connection pool is prewarmed before serving queries... then that seems like the thing we should give them.

All in all though... I wonder if adding a bounded timeout of something like 30 seconds by default would be sufficient here?

That seems better than waiting potentially forever.

I've added a 30 second timeout. for prefilling the pool. We'll see how that works out.

demmer · 2019-05-28T20:35:48Z

go/pools/resource_pool.go

-			rp.idleClosed.Add(1)
-			rp.active.Add(-1)
-		}
+		func() {


Was there some reason this needed to be wrapped in a function and not just included inline like it was before?

I find the extra layers of abstraction and the defer of putting the wrapper back into the resource pool to be more confusing this way.

It's necessary for panic-proofing. If Close or reopenResouce panicked, then this defensive code will prevent an outage. Otherwise, it will cause vttablet to lock up in many unexpected ways.

Ah that explains it 👍

demmer · 2019-05-28T20:42:39Z

go/pools/resource_pool.go

+			if wrapper.resource != nil && idleTimeout > 0 && time.Until(wrapper.timeUsed.Add(idleTimeout)) < 0 {
+				wrapper.resource.Close()
+				rp.idleClosed.Add(1)
+				rp.reopenResource(&wrapper)


I wonder if a cleaner implementation that would obviate the need for the manually refactored reopenResource would be for the idle closer to actually just call get and put internally.

This would require actually keeping the wait parameter that you removed, but it seems like changing this to call get to obtain the resource, then closing it, then calling put to return it back means that we only would need to do the reopen in one place.

I personally prefer the current approach because I've always been uncomfortable with the complexity of get. I was so happy to get rid of that wait. Now you want me to reintroduce it? :)

OK -- what if instead we didn't reintroduce wait, but instead we just call in with a short context deadline.
In the usual case we will immediately get the connection since the whole thing is going to only grab what it thinks is available.

Then we use ErrTimeout to indicate that we've gone through all we need to, instead of the "stop early" case above.

The main thing I don't like about this approach (both before and after your change) is that it duplicates a lot of the logic related to the stats and reopening the resource, etc.

If we use the existing get() / put() interface then closeIdleResources just operates like any other client to the pool -- it just grabs a connection to quickly close it, then rely on put() to reopen it and all the stats management would be done in get/put.

(I actually wish I had done that originally tbh)

Another related point which just came up in an internal discussion -- it would be nice to have vitess implement a clean mysql shutdown flow for the mysql protocol connections.

Currently our logs are filled up with things like:
2019-05-28T16:11:52.895744-08:00 163 [Note] Aborted connection 163 to db: 'vt_byuser' user: 'vt_app_a' host: 'localhost' (Got an error reading communication packets)

That's because Close() simply shuts down the underlying tcp socket.

I think instead we could bound the clean shutdown handshake by a relatively short context deadline (say 1-2 seconds) after which we summarily close the socket anyway.

That, to me, biases again for using the regular Get/Put interface for this.

It turns out that we can't use get here because it's the wrapper that has the timeUsed metadata. We could look at changing get to return the wrapper instead, or an empty one if no resource was available. WDYT?

Can we merge this in and do the refactor later if we still want it?

I think it'd be worthwhile to change get to return the possibly-nil-or-empty wrapper and thereby DRY this up and make the idle closer a bit cleaner.

demmer · 2019-05-28T20:44:53Z

go/pools/resource_pool_test.go

@@ -123,7 +123,7 @@ func TestOpen(t *testing.T) {
 	}
 	r.Close()
 	p.Put(nil)
-	if count.Get() != 4 {
+	if count.Get() != 5 {


The error message below doesn't match up with this expectation...
Also we should clarify with a comment that calling Put reopens the resource

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

tirsen · 2019-06-03T00:51:36Z

Only allocating the connections necessary.
This is the part of the requirements that seems to motivate a lot of the complexity in the other implementation and which I don't understand.

Can you explain the use case why you would want to allocate N connections up front, allowing a burst up to M, but where you're willing to pay the cost of waiting to connect for the burst spillover?

In staging we want to have multiple apps that share the same underlying physical database because Aurora clusters are expensive. Most of the time they are idle but we do want them to be able to burst every once in a while.

I think this is a valid use case but we can workaround it by simply allocating fewer apps to an Aurora cluster. Unfortunately this does make Vitess much more expensive than using MySQL without Vitess since the JDBC connection pool we're using (Hikari) does support this feature.

sougou · 2019-06-03T02:05:26Z

In staging we want to have multiple apps that share the same underlying physical database because Aurora clusters are expensive. Most of the time they are idle but we do want them to be able to burst every once in a while.

I think this is a valid use case but we can workaround it by simply allocating fewer apps to an Aurora cluster. Unfortunately this does make Vitess much more expensive than using MySQL without Vitess since the JDBC connection pool we're using (Hikari) does support this feature.

I didn't know about this requirement. If this is the case, we need a different approach. The reason is because we need a way to shrink the pool back to a smaller size when things go idle. Otherwise, you'll eventually run out of connections as each app bursts beyond the preallocated limit.

The original conn-pool implementation used to prefer reusing available connections. But we found out it was production-unfriendly because things broke unexpectedly during bursts. So, we changed it to always fill the pool.

But it makes sense for a testing setup. I'm thinking we could dynamically expand or shrink the pool size by calling SetCapacity based on certain criteria.

tirsen · 2019-06-03T06:28:57Z

I didn't know about this requirement. If this is the case, we need a different approach. The reason is because we need a way to shrink the pool back to a smaller size when things go idle. Otherwise, you'll eventually run out of connections as each app bursts beyond the preallocated limit.

We don't need to solve this now. What we have here solves most of our most pressing needs. Shrinking is optimizing for cost but that's of lower priority right now.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

mpawliszyn · 2019-06-07T15:52:21Z

go/pools/resource_pool.go

@@ -37,6 +37,8 @@ var (

 	// ErrTimeout is returned if a resource get times out.
 	ErrTimeout = errors.New("resource pool timed out")
+
+	prefillTimeout = 30 * time.Second


May not be enough based on our testing. On a cold start opening 600 connections takes about ~38s.

So maybe make this configurable?

🤷‍♂️ Yet another flag. I'll begrudgingly add it.

I know this is getting to be a lot, but I just don't see how we can make a one-size-fits-all solution here.

I'd say let's go with 30 seconds for now. If it actually becomes a problem in real life then we can address it by making it configurable then.

demmer

Overall I'm satisfied with this as-is.

sougou force-pushed the ss-prefilled-pool branch from 968ec28 to a0ae7d7 Compare March 31, 2019 16:50

demmer mentioned this pull request Apr 25, 2019

Experimental "fast pool". #4697

Closed

sougou added 3 commits May 24, 2019 05:14

pools: solidfy prefill code and add tests

ba54ddc

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

pools: tabletserver prefill cmd line options

1bbe3cc

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou marked this pull request as ready for review May 25, 2019 13:47

sougou force-pushed the ss-prefilled-pool branch from a0ae7d7 to 1bbe3cc Compare May 25, 2019 13:49

pools: unify constructor to NewResourcePool

9cebb40

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

mpawliszyn reviewed May 28, 2019

View reviewed changes

mpawliszyn approved these changes May 28, 2019

View reviewed changes

demmer reviewed May 28, 2019

View reviewed changes

pools: address review comments

94a33b4

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

pools: address more review comments

61655c0

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

mpawliszyn reviewed Jun 7, 2019

View reviewed changes

demmer reviewed Jun 17, 2019

View reviewed changes

sougou merged commit f25720a into vitessio:master Jun 17, 2019

sougou deleted the ss-prefilled-pool branch June 17, 2019 23:58

pools: option to prefill #4769

pools: option to prefill #4769

Conversation

sougou commented Mar 31, 2019

sougou commented Mar 31, 2019

tirsen commented Mar 31, 2019

sougou commented Apr 1, 2019

demmer commented Apr 25, 2019

sougou commented May 25, 2019

sougou commented May 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpawliszyn 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

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

mpawliszyn Jun 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tirsen commented Jun 3, 2019

sougou commented Jun 3, 2019

tirsen commented Jun 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demmer left a comment

Choose a reason for hiding this comment

mpawliszyn Jun 13, 2019 •

edited