fishy commented on a change in pull request #2497: URL: https://github.com/apache/thrift/pull/2497#discussion_r794001034
########## File path: lib/go/README.md ########## @@ -132,3 +132,28 @@ if this interval is set to a value too low (for example, 1ms), it might cause excessive cpu overhead. This feature is also only enabled on non-oneway endpoints. + +A note about server stop implementations +======================================== + +[TSimpleServer.Stop](https://pkg.go.dev/github.com/apache/thrift/lib/go/thrift#TSimpleServer.Stop) will wait for all client connections to be closed after +the last received or to be received request to be handled, so the time spent by stop + may be too long in some situation: +* when socket timeout is not set,server will be hanged before all active Review comment: nit: add a space after `,` (and any other punctuation): `s/set,server/set, server/` ########## File path: lib/go/README.md ########## @@ -132,3 +132,28 @@ if this interval is set to a value too low (for example, 1ms), it might cause excessive cpu overhead. This feature is also only enabled on non-oneway endpoints. + +A note about server stop implementations +======================================== + +[TSimpleServer.Stop](https://pkg.go.dev/github.com/apache/thrift/lib/go/thrift#TSimpleServer.Stop) will wait for all client connections to be closed after +the last received or to be received request to be handled, so the time spent by stop + may be too long in some situation: +* when socket timeout is not set,server will be hanged before all active + clients to finish handling the last received or to be received request. +* when the socket timeout is too long saying one hour, server will be + hanged for one hour before all active clients to finish handling the + last received or to be received request. + +To avoid the server stop to be hanged for too long, you can take +advantage of thrift.ServerStopTimeout in your main or init function: + + thrift.ServerStopTimeout = <max_duration_to_stop> + +If it's set to <=0, the feature will be disabled(by default), and server Review comment: nit: add space before `(`, also I _think_ from grammar perspective it should be just "(default)" instead of "(by default)". ########## File path: lib/go/README.md ########## @@ -132,3 +132,28 @@ if this interval is set to a value too low (for example, 1ms), it might cause excessive cpu overhead. This feature is also only enabled on non-oneway endpoints. + +A note about server stop implementations +======================================== + +[TSimpleServer.Stop](https://pkg.go.dev/github.com/apache/thrift/lib/go/thrift#TSimpleServer.Stop) will wait for all client connections to be closed after +the last received or to be received request to be handled, so the time spent by stop + may be too long in some situation: +* when socket timeout is not set,server will be hanged before all active + clients to finish handling the last received or to be received request. +* when the socket timeout is too long saying one hour, server will be + hanged for one hour before all active clients to finish handling the + last received or to be received request. + +To avoid the server stop to be hanged for too long, you can take +advantage of thrift.ServerStopTimeout in your main or init function: + + thrift.ServerStopTimeout = <max_duration_to_stop> + +If it's set to <=0, the feature will be disabled(by default), and server +will wait for all the client connections to be closed gracefully with +zero err time.Otherwise, the stop will wait for all the client +connections to be closed gracefully util thrift.ServerStopTimeout is +reached, and the client connections that are not closed after thrift. Review comment: don't put line break after `thrift.` as that will cause a space being added (so it's rendered as `thrift. ServerStopTimeout`). ########## File path: lib/go/README.md ########## @@ -132,3 +132,28 @@ if this interval is set to a value too low (for example, 1ms), it might cause excessive cpu overhead. This feature is also only enabled on non-oneway endpoints. + +A note about server stop implementations +======================================== + +[TSimpleServer.Stop](https://pkg.go.dev/github.com/apache/thrift/lib/go/thrift#TSimpleServer.Stop) will wait for all client connections to be closed after +the last received or to be received request to be handled, so the time spent by stop + may be too long in some situation: +* when socket timeout is not set,server will be hanged before all active + clients to finish handling the last received or to be received request. +* when the socket timeout is too long saying one hour, server will be + hanged for one hour before all active clients to finish handling the + last received or to be received request. + +To avoid the server stop to be hanged for too long, you can take +advantage of thrift.ServerStopTimeout in your main or init function: + + thrift.ServerStopTimeout = <max_duration_to_stop> + +If it's set to <=0, the feature will be disabled(by default), and server +will wait for all the client connections to be closed gracefully with +zero err time.Otherwise, the stop will wait for all the client +connections to be closed gracefully util thrift.ServerStopTimeout is +reached, and the client connections that are not closed after thrift. +ServerStopTimeout will be closed abruptly which may cause some client err +rate. Review comment: "cause client errors", you can't cause a "err rate" :) ########## File path: lib/go/README.md ########## @@ -132,3 +132,28 @@ if this interval is set to a value too low (for example, 1ms), it might cause excessive cpu overhead. This feature is also only enabled on non-oneway endpoints. + +A note about server stop implementations +======================================== + +[TSimpleServer.Stop](https://pkg.go.dev/github.com/apache/thrift/lib/go/thrift#TSimpleServer.Stop) will wait for all client connections to be closed after +the last received or to be received request to be handled, so the time spent by stop + may be too long in some situation: +* when socket timeout is not set,server will be hanged before all active + clients to finish handling the last received or to be received request. +* when the socket timeout is too long saying one hour, server will be Review comment: 1. `s/saying/say/` 2. `s/will/might/`, also it should be "be hanging" instead of "be hanged", I think? ########## File path: lib/go/README.md ########## @@ -132,3 +132,28 @@ if this interval is set to a value too low (for example, 1ms), it might cause excessive cpu overhead. This feature is also only enabled on non-oneway endpoints. + +A note about server stop implementations +======================================== + +[TSimpleServer.Stop](https://pkg.go.dev/github.com/apache/thrift/lib/go/thrift#TSimpleServer.Stop) will wait for all client connections to be closed after +the last received or to be received request to be handled, so the time spent by stop + may be too long in some situation: +* when socket timeout is not set,server will be hanged before all active + clients to finish handling the last received or to be received request. +* when the socket timeout is too long saying one hour, server will be + hanged for one hour before all active clients to finish handling the + last received or to be received request. + +To avoid the server stop to be hanged for too long, you can take +advantage of thrift.ServerStopTimeout in your main or init function: + + thrift.ServerStopTimeout = <max_duration_to_stop> + +If it's set to <=0, the feature will be disabled(by default), and server +will wait for all the client connections to be closed gracefully with +zero err time.Otherwise, the stop will wait for all the client Review comment: `s/time.Otherwise/time. Otherwise/` -- 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]
