buptubuntu commented on pull request #2497: URL: https://github.com/apache/thrift/pull/2497#issuecomment-1011213651
> Wait. Sorry I just realized that just closing all clients during `TSimpleServer.Stop` would be a behavior change and make things worse. > > In the current code, when `TSimpleServer.Stop` is called, it's a graceful shutdown: it makes `TSimpleServer` to stop accepting new connections (via the atomic int and `Interrupt` call), but it still waits for any request that's currently handling to finish. And letting any request that's currently handling to finish is an important part of graceful shutdown. > > Now we just close all the connections (this is true with either the goroutine approach or original map approach), which will cause any currently handling connection to be closed abruptly. from client's pov, they will not receive the full response for this request (it could be either they haven't received any response at all, or they received half of the response then the unexpected EOF happens), and this will cause those requests to fail (because we didn't let them to finish). > > So when any server upgrade to this version, this will increase their error rate whenever autoscaling is scaling down. This is a worse scenario than graceful shutdown just timeouts (kubernnetes has a timeout to wait for graceful shutdown, so in current version as long as the server can finish all the pending requests in that time they won't fail any requests just because of a shutdown happens). > > I don't know what's the best way to fix that. Maybe add a setting to `TSimpleServer` to sleep for a period of time before trying to abruptly close all connections is a ok short term solution. for long term we might need to make some breaking changes to `TProtocol`/`TTransport` to add the graceful shutdown semantic to them. > > @dcelasun your thoughts? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
