[vtadmin] cluster rpc pools #8421
Conversation
An RPCPool is a special case of the ResourcePool, where the actual "resources" in the pool are essentially a lease to perform an RPC (or access any shared resource) with differing timeout semantics than the sync2.Semaphore. Signed-off-by: Andrew Mason <[email protected]>
We're about to get many more cases, so I'm simplifying it now. Signed-off-by: Andrew Mason <[email protected]>
Signed-off-by: Andrew Mason <[email protected]>
Signed-off-by: Andrew Mason <[email protected]>
Signed-off-by: Andrew Mason <[email protected]>
ajm188
added
Type: Enhancement
| func (pool *RPCPool) Acquire(ctx context.Context) error { | ||
| if pool.waitTimeout > 0 { | ||
| var cancel context.CancelFunc | ||
| ctx, cancel = context.WithTimeout(ctx, pool.waitTimeout) |
There was a problem hiding this comment.
Does context.WithTimeout automatically respect the lesser of the given timeout or the ctx (first parameter)'s timeout? 🤔 I think I am missing something since the docs say:
// WithTimeout returns WithDeadline(parent, time.Now().Add(timeout)).
There was a problem hiding this comment.
Yes, you can't extend a timeout in a context, only shorten it.
There was a problem hiding this comment.
Yeah, it's in the docs for WithDeadline:
WithDeadline returns a copy of the parent context with the deadline adjusted to be no later than d. If the parent's deadline is already earlier than d, WithDeadline(parent, d) is semantically equivalent to parent.
| Cells: cells, | ||
| }) | ||
| c.topoReadPool.Release() |
There was a problem hiding this comment.
Non-blocking question -- in some cases, we use defer c.topoReadPool.Relase() and in others, like here, we Acquire and the Release sequentially (for lack of a better term).
In this case, it would be equivalent to defer, since it's the only Acquire/Release in the function, right? Is there much of a difference? (You've kinda sold me on the defer way since it seems easy (for me) to forget, otherwise, so I'm curious. 😈)
There was a problem hiding this comment.
This is a general comment not directed at this specific usage. Until Go 1.14, defer had a significant penalty for hot paths, so you saw a lot of people avoiding it. That has mostly been eliminated, so except for rare circumstances, defer is the safer choice. IMO, it's easier to read, plus eliminates the possibility of adding a return somewhere without handling the unlock.
https://rakyll.org/inlined-defers/
There was a problem hiding this comment.
Yeah, that's exactly the intent I had behind this. If we're just making the rpc, and moving the result from one struct to another (so no other function calls, or iterating over elements in a response, etc), then it's "fine" to just defer the release, but if we're going to do any other "meaningful" work (which i am loosely defining as function calls or iterations) then we should release immediately after finishing the rpc to free up other waiting goroutines.
There was a problem hiding this comment.
Ah somehow ... github did not show me Derek's comment. Derek's completely correct, but there's another consideration in that we are not just making the rpc call, so it doesn't make sense to be holding onto a slot in the pool for us to do unrelated work.
Description
This adds an RPCPool specialization on top of the generic pools.ResourcePool, and then adds several read pools (named this way so we don't need to rename (and rename the flags) when we add the r/w pools later). The main reason I'm not using sync2.Semaphore is because the timeout/context semantics don't work for our use case. With the semaphore, you can either use the semaphore's timeout or the context timeout, but what I want is to always use the minimum of the two. RPCPool does exactly that, plus we get all the stats/metrics of ResourcePool "for free".
From there, we add 4 rpc pools:
Related Issue(s)
Checklist
Deployment Notes