GoogleCloudPlatform / grpc-gcp-csharp

Apache License 2.0
14 stars 9 forks source link

Allow bindings to expire - or other pool management options #15

Open jskeet opened 6 years ago

jskeet commented 6 years ago

Currently there's no bound on the affinity key mapping (as far as I can see). While client code should try to clean up after itself appropriately, I can imagine cases where that may be hard to achieve. It would be good to have some way of either explicitly or implicitly expiring these keys.

One area where I'm currently unsure of what happens is if an unbinding call fails:

These two points have contradictory implications in terms of what the call invoker should do...

WeiranFang commented 6 years ago

Thanks for pointing that out! I haven't thought of the retry scenarios from client.

As far as I know (correct me if I'm wrong), the channel affinity is only used for caching or improving low latency when making querying requests. So I think even we remove the affinity before client making another BOUND or UNBIND requests, the request itself won't fail with a different channel. So I don't think it's a hard requirement to use the same channel when client makes affinity requests. (For example, in Spanner, as long as we provide a correct session name, the DeleteSession call won't fail just because we are using another channel, right?)

In this case, I think we can clean the affinity mapping for UNBIND calls without considering the response being successful or not. In another word, we clean the mapping as long as we have an UNBIND request. When client wants to do retry, it is OK to use another channel to make the retry request, right? WDYT?

jskeet commented 6 years ago

As far as I know (correct me if I'm wrong), the channel affinity is only used for caching or improving low latency when making querying requests.

I'm not sure on this. We were told pretty clearly to make sure we used the same channel for all Spanner requests. I wouldn't be surprised if DeleteSession were an exception to this though. I'll check.

WeiranFang commented 6 years ago

Thanks. If DeleteSession is the only exception, then what do you think if we add a flag to track the number of failure retries for unbind requests?

jskeet commented 6 years ago

I'm unsure as yet. I'll let you know once we have more information.

jskeet commented 6 years ago

Have received word back - in general, the channel affinity improves latency and reduces the chances or aborted transactions. In the case of DeleteSession the aborted transactions part is irrelevant and latency isn't much of a problem either - so we're fine.

I'll leave this issue open for the moment for the broader issue of an ever-growing affinity key dictionary.