spring-projects-experimental / spring-grpc

88 stars 19 forks source link

Support for shutdown grace period in netty clients #54

Open panic08 opened 1 week ago

panic08 commented 1 week ago

This PR refers to #12

Here we give opportunity to set a specific shutdown grace period in properties. If the customer channel is not fulfilled within this time, it will be shutdowned now.

dsyer commented 1 week ago

Looks good. Two questions (both could be deferred to future work):

1) Would it be a good idea to try and shutdown all the clients in parallel, rather than waiting for each one?

2) Couldn't each channel have its own timeout? It looks like the model you made allows it, but then ignores them. If it's too complicated we'd either need a new abstraction in core, or we could pull the global timeout up out of the NamedChannel.

panic08 commented 1 week ago

Looks good. Two questions (both could be deferred to future work):

1) Would it be a good idea to try and shutdown all the clients in parallel, rather than waiting for each one?

2) Couldn't each channel have its own timeout? It looks like the model you made allows it, but then ignores them. If it's too complicated we'd either need a new abstraction in core, or we could pull the global timeout up out of the NamedChannel.

I think it's a good idea to develop in the future. I guess the grace period for the client implies a grace period for each channel

However, making a grace period for all channels simultaneously and for individual channels is a good idea that can be developed in the future

Perhaps this can be put into two separate parameters as well

I suppose that as you said, in the channel factory it will be possible to delete all channels with 1 timeout in parallel in the future. We can think about deleting each channel with timeout, where it is worth to implement this

dsyer commented 1 week ago

I'm a bit worried that leaving the timeout as a NamedChannel property makes it obligatory to implement individual timeouts at some point. So I'd like to know if it's awkward or impossible at least, in case we don't want to do it. I can have a think but if you have any insight please share it.

panic08 commented 1 week ago

We can now rename shutdownGracePeriod to shutdownGracePeriodPerRequest, thus ensuring backward compatibility and then we won't have to worry if (and I guess when) we want to do a parallel shutdown grace period

dsyer commented 1 week ago

I don't understand what "per request" means. Isn't it "per channel" (in which case the fact that it is a property of a NamedChannel makes it redundant)?

panic08 commented 1 week ago

Yes, I apologize, I mean “perChannel”.

So the current shutdownGracePeriod that I created we will rename directly now to shutdownGracePeriodPerChannel. This will solve any problems that may arise if we decide to create a parallel shutdownGracePeriod

dsyer commented 1 week ago

OK, then the name doesn't need to change (because it's a property of a channel already). I don't think that solves any problems. Still thinking. If you have any ideas about how to implement the "per channel" behaviour then feel free to try them out.

panic08 commented 1 week ago

I assume that by “per channel” we mean setting a specific shutdownGracePeriod separately for each channel? Is it possible to theriotically create some kind of settings class for channels to solve this issue

We could overload the createChannel method in GrpcChannelFactory and add another argument to the method with the settings class for the channel. Then directly in the DefaultGrpcChannelFactory we could monitor if the setting for that channel is present with the shutdownGracePeriod field, and if so, we use it for that channel in the destroy method. If not, we use the default one set in properties (or if not in properties - which we set systematically)

That is, my main point is to introduce a class for the settings for the channel to be set when creating the channel in createChannel