LGTM

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @jaylim-crl, and @jeffswenson)

pkg/ccl/sqlproxyccl/proxy_handler.go, line 180 at r4 (raw file):

	}

	ctx, _ = stopper.WithCancelOnQuiesce(ctx)

Should WithCancelOnQuiesce be called before we use ctx in the NewCertManager call? It's strange that we use different ctx instances in different places.

pkg/ccl/sqlproxyccl/balancer/balancer.go, line 117 at r4 (raw file):

	}

	if err := b.stopper.RunAsyncTask(ctx, "processQueue-closer", func(ctx context.Context) {

How come we need this separate async task? I thought that ctx.Done would be closed when the stopper is quiesced, and therefore we could close the queue in processQueue at that point.

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @jeffswenson)

pkg/ccl/sqlproxyccl/proxy_handler.go, line 180 at r4 (raw file):

Yes, we could. We would need to thread ctx to setupIncomingCert. Also, that's already an existing issue today. I also don't see why we'd need a separate context for the cert manager. I can do that here.

pkg/ccl/sqlproxyccl/balancer/balancer.go, line 117 at r4 (raw file):

and therefore we could close the queue in processQueue at that point.

The first part is correct, but the second isn't the case. The queue has no notion of context.Context, and there's nothing to wake the callers up whenever ctx.Done has been closed. The ctx object in processQueue is only used to indicate whether we want to continue reading from the queue. When we get blocked when reading from the queue, someone would need to invoke queue.close() explicitly to wake those callers up.

pkg/ccl/sqlproxyccl/balancer/balancer.go, line 152 at r4 (raw file):

Good point. I can make this change.

pkg/ccl/sqlproxyccl/balancer/balancer_test.go, line 163 at r4 (raw file):

Hm, let me look into this again. Maybe there's a simpler approach.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @jeffswenson)

pkg/ccl/sqlproxyccl/proxy_handler.go, line 180 at r4 (raw file):

Done.

pkg/ccl/sqlproxyccl/balancer/balancer.go, line 152 at r4 (raw file):

Done. Actually, I see various approaches:

1. chan struct{}, e.g.:

   cockroach/pkg/kv/kvserver/store.go

   Lines 765 to 766 in 63ea913

   	// Semaphore to limit concurrent non-empty snapshot application.
   	snapshotApplySem chan struct{}

2. marusama/semaphore
3. quotapool.IntPool

Regardless, I've updated this to use (2) since it's cleaner.

pkg/ccl/sqlproxyccl/balancer/balancer_test.go, line 163 at r4 (raw file):

I left it as-is. I still need the afterProcessQueueItem hook for other sub-tests.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @jaylim-crl)

pkg/ccl/sqlproxyccl/balancer/balancer.go, line 117 at r4 (raw file):

One idea: we could use a semaphore for tracking the size of the queue instead of the condition variable. That allows us to drop the close state and the goroutine. The implementation would look like:

func (q Queue) Push(item interface{}) {
  q.Lock()
  defer q.Unlock()
  // add the item to the queue
  q.semaphore.Release(1)
}

func (q Queue) Dequeue(ctx context.Context) (item interface{}, err error) {
  if err := q.semaphore.Acquire(ctx, 1); err != nil {
    return err
  }
  q.Lock()
  defer q.Unlock()
  // remove element from the queue
}

pkg/ccl/sqlproxyccl/balancer/balancer_test.go, line 163 at r4 (raw file):

It is possible to avoid the need for the afterProcessQueueItem hook. When setting eventCh<-, we are using it to determine when processing is complete. The test can wait for processing to complete by limiting the concurrency to 1 and publishing a second event that closes eventCh.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @jaylim-crl)

pkg/ccl/sqlproxyccl/balancer/balancer.go, line 117 at r4 (raw file):

I've done something like that before, but if we'd like to stick to github.com/marusama/semaphore, the above won't work for two reasons:

1. We need a size limit for that to work.
2. Release panics without Acquire: https://github.com/marusama/semaphore/blob/2d3c1eaa054b6e36c7c0dfde398f2b47e4bc5094/semaphore.go#L169-L171.

Does that align with what you think as well?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @jaylim-crl)

pkg/ccl/sqlproxyccl/balancer/balancer.go, line 117 at r4 (raw file):

During initialization you can set the capacity to something really large and then acquire it all. It looks like the easiest way to do that with the marusama semaphore is:

semaphore := semaphore.New(0)
semaphore.SetLimit(MaxUint32)

TFTR! I'll come back again with an updated queue implementation.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball)

pkg/ccl/sqlproxyccl/balancer/balancer.go, line 117 at r4 (raw file):

😄 I like that idea. I'll rework the queue as I think it's much more ergonomic being able to unblock when ctx is cancelled automatically.

pkg/ccl/sqlproxyccl/balancer/balancer_test.go, line 163 at r4 (raw file):

Done.

Done. Everything has been addressed :) dequeue now reacts to context cancellations, and will be unblocked automatically when that happens.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball)

pkg/ccl/sqlproxyccl/balancer/balancer.go, line 117 at r4 (raw file):

Done.