-
-
Notifications
You must be signed in to change notification settings - Fork 15.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SimpleChannelPool/FixedChannelPool: Don't put unhealthy channel back to the pool on release #4077
Comments
@normanmaurer - I recall discussing this when we originally reviewed the pool. I think your rational was since we have to check the health anyways when we pull out of the pool, and the health check may be expensive (may include RTT), we wanted to minimize the number of health checks done. WDYT? |
@Scottmitch yep that was my argument, I may been wrong here and we should kind of make this configurable. WDYT ? |
@normanmaurer yes. As this issue describes I think there is value in having the option to check up front too. |
Motivation: When releasing unhealthy channel back to a pool we don't have to offer it since on acquire it will be discarded anyways. Also checking healthiness at release is a good idea so we don't end up having tons of unhealthy channels in the pool(unless they became unhealthy after being offered) Modifications: private SimpleChannelPool.offerIfHealthy() method added that is called from SimpleChannelPool.doReleaseChannel(). SimpleChannelPool.offerIfHealthy() offers channel back to pool only if channel is healthy. Otherwise it throws setFailure exception to the promise. Result: The pool is now much cleaner and not spammed with unhealthy channels. Added ability to choose if channel health has to be validated on release by passing boolean flag. Motivation: Depending on performance preferences and individual use cases sometimes we would like to be able force health check of a channel at release time and do not offer it back to the pool. Other times we would want to just release channel and offer it back to the pool and check health only when we try to acquire that channel from the pool. See more details here: netty#4077 (comment) Modifications: Future<Void> release(Channel channel, Promise<Void> promise, boolean offerHealthyOnly); The offerHealthyOnly boolean flag allows developers to choose whether to do channel validation before offering it back to pool or not. Appropriate modifications made to hierarchy of implementations of ChannelPool. offerHealthyOnly=true will force channel health to be checked before offering back to pool. offerHealthyOnly=false will ignore channel health check and will just try just offer it back to the pool offerHealthyOnly=true by default. Result: Channel health check before offer back to pool is controlled by a flag now. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:279 line split to be less then 120 characters. SimpleChannelPool.java:280:31 space added after '{' SimpleChannelPool.java:282:17 space added after '{' SimpleChannelPoolTest.java:198 - extra white space line removed. Result: Code satisfies checkstyle requirements. offerHealthyOnly is passed as a constructor parameter now. Motivation: Instead of passing offerHealthyOnly as a method parameter it is better to pass it in as SimpleChannelPool or FixedChannelPool constructor. Modifications: Redundant release method that takes offerHealthyOnly removed from ChannelPool. offerHealthyOnly parameter added to constructor for FixedChannelPool and SimpleChannelPool. Result: SimpleChannelPool and FixedChannelPool are now take offerHealthyOnly as a constructor parameter. Default behavior is: offerHealthyOnly=true. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:84: line made to be no longer then 120 characters. SimpleChannelPool.java:237: extra white space line removed. Result: Code satisfies checkstyle requirements. Tests do not need to be too copled to the code. Exception message should not be validated Motivation: We don't need our tests to be too coupled to the code. Exception type validation in tests is just good enough. Modifications: Exception validation message removed from SimpleChannelPoolTest.testUnhealthyChannelIsNotOffered() test. Result: The SimpleChannelPoolTest test is less coupled to the code now. Stack trace set to empty for UNHEALTHY_NON_OFFERED_TO_POOL. Motivation: We don't need stack trace for UNHEALTHY_NON_OFFERED_TO_POOL. Modifications: Added UNHEALTHY_NON_OFFERED_TO_POOL.setStackTrace(EmptyArrays.EMPTY_STACK_TRACE) to static init block. Result: UNHEALTHY_NON_OFFERED_TO_POOL's stack trace set to empty. Minor code re-factorings. Motivation: For better code readability we need to apply several minor code re-factorings. Modifications: javadocs true -> {@code true} offerHealthyOnly variable name changed to releaseHeathCheck <p/> -> <p> in javadocs offerHealthyOnly removed from doReleaseChannel as it not needed there. Result: Code quality is improved. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:87: line made to be no longer then 120 characters. Result: Code satisfies checkstyle requirements. Pull request needs to contain only necessary changes Motivation: The pull request should not contain unnecessary changes that are not needed as part of required functionality of pull request. Modifications: private void doReleaseChannel(final Channel channel, final Promise<Void> promise) - > private void doReleaseChannel(Channel channel, Promise<Void> promise) Result: Pull request contains less unnecessary modifications.
Motivation: When releasing unhealthy channel back to a pool we don't have to offer it since on acquire it will be discarded anyways. Also checking healthiness at release is a good idea so we don't end up having tons of unhealthy channels in the pool(unless they became unhealthy after being offered) Modifications: private SimpleChannelPool.offerIfHealthy() method added that is called from SimpleChannelPool.doReleaseChannel(). SimpleChannelPool.offerIfHealthy() offers channel back to pool only if channel is healthy. Otherwise it throws setFailure exception to the promise. Result: The pool is now much cleaner and not spammed with unhealthy channels. Added ability to choose if channel health has to be validated on release by passing boolean flag. Motivation: Depending on performance preferences and individual use cases sometimes we would like to be able force health check of a channel at release time and do not offer it back to the pool. Other times we would want to just release channel and offer it back to the pool and check health only when we try to acquire that channel from the pool. See more details here: #4077 (comment) Modifications: Future<Void> release(Channel channel, Promise<Void> promise, boolean offerHealthyOnly); The offerHealthyOnly boolean flag allows developers to choose whether to do channel validation before offering it back to pool or not. Appropriate modifications made to hierarchy of implementations of ChannelPool. offerHealthyOnly=true will force channel health to be checked before offering back to pool. offerHealthyOnly=false will ignore channel health check and will just try just offer it back to the pool offerHealthyOnly=true by default. Result: Channel health check before offer back to pool is controlled by a flag now. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:279 line split to be less then 120 characters. SimpleChannelPool.java:280:31 space added after '{' SimpleChannelPool.java:282:17 space added after '{' SimpleChannelPoolTest.java:198 - extra white space line removed. Result: Code satisfies checkstyle requirements. offerHealthyOnly is passed as a constructor parameter now. Motivation: Instead of passing offerHealthyOnly as a method parameter it is better to pass it in as SimpleChannelPool or FixedChannelPool constructor. Modifications: Redundant release method that takes offerHealthyOnly removed from ChannelPool. offerHealthyOnly parameter added to constructor for FixedChannelPool and SimpleChannelPool. Result: SimpleChannelPool and FixedChannelPool are now take offerHealthyOnly as a constructor parameter. Default behavior is: offerHealthyOnly=true. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:84: line made to be no longer then 120 characters. SimpleChannelPool.java:237: extra white space line removed. Result: Code satisfies checkstyle requirements. Tests do not need to be too copled to the code. Exception message should not be validated Motivation: We don't need our tests to be too coupled to the code. Exception type validation in tests is just good enough. Modifications: Exception validation message removed from SimpleChannelPoolTest.testUnhealthyChannelIsNotOffered() test. Result: The SimpleChannelPoolTest test is less coupled to the code now. Stack trace set to empty for UNHEALTHY_NON_OFFERED_TO_POOL. Motivation: We don't need stack trace for UNHEALTHY_NON_OFFERED_TO_POOL. Modifications: Added UNHEALTHY_NON_OFFERED_TO_POOL.setStackTrace(EmptyArrays.EMPTY_STACK_TRACE) to static init block. Result: UNHEALTHY_NON_OFFERED_TO_POOL's stack trace set to empty. Minor code re-factorings. Motivation: For better code readability we need to apply several minor code re-factorings. Modifications: javadocs true -> {@code true} offerHealthyOnly variable name changed to releaseHeathCheck <p/> -> <p> in javadocs offerHealthyOnly removed from doReleaseChannel as it not needed there. Result: Code quality is improved. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:87: line made to be no longer then 120 characters. Result: Code satisfies checkstyle requirements. Pull request needs to contain only necessary changes Motivation: The pull request should not contain unnecessary changes that are not needed as part of required functionality of pull request. Modifications: private void doReleaseChannel(final Channel channel, final Promise<Void> promise) - > private void doReleaseChannel(Channel channel, Promise<Void> promise) Result: Pull request contains less unnecessary modifications.
Motivation: When releasing unhealthy channel back to a pool we don't have to offer it since on acquire it will be discarded anyways. Also checking healthiness at release is a good idea so we don't end up having tons of unhealthy channels in the pool(unless they became unhealthy after being offered) Modifications: private SimpleChannelPool.offerIfHealthy() method added that is called from SimpleChannelPool.doReleaseChannel(). SimpleChannelPool.offerIfHealthy() offers channel back to pool only if channel is healthy. Otherwise it throws setFailure exception to the promise. Result: The pool is now much cleaner and not spammed with unhealthy channels. Added ability to choose if channel health has to be validated on release by passing boolean flag. Motivation: Depending on performance preferences and individual use cases sometimes we would like to be able force health check of a channel at release time and do not offer it back to the pool. Other times we would want to just release channel and offer it back to the pool and check health only when we try to acquire that channel from the pool. See more details here: #4077 (comment) Modifications: Future<Void> release(Channel channel, Promise<Void> promise, boolean offerHealthyOnly); The offerHealthyOnly boolean flag allows developers to choose whether to do channel validation before offering it back to pool or not. Appropriate modifications made to hierarchy of implementations of ChannelPool. offerHealthyOnly=true will force channel health to be checked before offering back to pool. offerHealthyOnly=false will ignore channel health check and will just try just offer it back to the pool offerHealthyOnly=true by default. Result: Channel health check before offer back to pool is controlled by a flag now. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:279 line split to be less then 120 characters. SimpleChannelPool.java:280:31 space added after '{' SimpleChannelPool.java:282:17 space added after '{' SimpleChannelPoolTest.java:198 - extra white space line removed. Result: Code satisfies checkstyle requirements. offerHealthyOnly is passed as a constructor parameter now. Motivation: Instead of passing offerHealthyOnly as a method parameter it is better to pass it in as SimpleChannelPool or FixedChannelPool constructor. Modifications: Redundant release method that takes offerHealthyOnly removed from ChannelPool. offerHealthyOnly parameter added to constructor for FixedChannelPool and SimpleChannelPool. Result: SimpleChannelPool and FixedChannelPool are now take offerHealthyOnly as a constructor parameter. Default behavior is: offerHealthyOnly=true. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:84: line made to be no longer then 120 characters. SimpleChannelPool.java:237: extra white space line removed. Result: Code satisfies checkstyle requirements. Tests do not need to be too copled to the code. Exception message should not be validated Motivation: We don't need our tests to be too coupled to the code. Exception type validation in tests is just good enough. Modifications: Exception validation message removed from SimpleChannelPoolTest.testUnhealthyChannelIsNotOffered() test. Result: The SimpleChannelPoolTest test is less coupled to the code now. Stack trace set to empty for UNHEALTHY_NON_OFFERED_TO_POOL. Motivation: We don't need stack trace for UNHEALTHY_NON_OFFERED_TO_POOL. Modifications: Added UNHEALTHY_NON_OFFERED_TO_POOL.setStackTrace(EmptyArrays.EMPTY_STACK_TRACE) to static init block. Result: UNHEALTHY_NON_OFFERED_TO_POOL's stack trace set to empty. Minor code re-factorings. Motivation: For better code readability we need to apply several minor code re-factorings. Modifications: javadocs true -> {@code true} offerHealthyOnly variable name changed to releaseHeathCheck <p/> -> <p> in javadocs offerHealthyOnly removed from doReleaseChannel as it not needed there. Result: Code quality is improved. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:87: line made to be no longer then 120 characters. Result: Code satisfies checkstyle requirements. Pull request needs to contain only necessary changes Motivation: The pull request should not contain unnecessary changes that are not needed as part of required functionality of pull request. Modifications: private void doReleaseChannel(final Channel channel, final Promise<Void> promise) - > private void doReleaseChannel(Channel channel, Promise<Void> promise) Result: Pull request contains less unnecessary modifications.
Motivation: When releasing unhealthy channel back to a pool we don't have to offer it since on acquire it will be discarded anyways. Also checking healthiness at release is a good idea so we don't end up having tons of unhealthy channels in the pool(unless they became unhealthy after being offered) Modifications: private SimpleChannelPool.offerIfHealthy() method added that is called from SimpleChannelPool.doReleaseChannel(). SimpleChannelPool.offerIfHealthy() offers channel back to pool only if channel is healthy. Otherwise it throws setFailure exception to the promise. Result: The pool is now much cleaner and not spammed with unhealthy channels. Added ability to choose if channel health has to be validated on release by passing boolean flag. Motivation: Depending on performance preferences and individual use cases sometimes we would like to be able force health check of a channel at release time and do not offer it back to the pool. Other times we would want to just release channel and offer it back to the pool and check health only when we try to acquire that channel from the pool. See more details here: #4077 (comment) Modifications: Future<Void> release(Channel channel, Promise<Void> promise, boolean offerHealthyOnly); The offerHealthyOnly boolean flag allows developers to choose whether to do channel validation before offering it back to pool or not. Appropriate modifications made to hierarchy of implementations of ChannelPool. offerHealthyOnly=true will force channel health to be checked before offering back to pool. offerHealthyOnly=false will ignore channel health check and will just try just offer it back to the pool offerHealthyOnly=true by default. Result: Channel health check before offer back to pool is controlled by a flag now. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:279 line split to be less then 120 characters. SimpleChannelPool.java:280:31 space added after '{' SimpleChannelPool.java:282:17 space added after '{' SimpleChannelPoolTest.java:198 - extra white space line removed. Result: Code satisfies checkstyle requirements. offerHealthyOnly is passed as a constructor parameter now. Motivation: Instead of passing offerHealthyOnly as a method parameter it is better to pass it in as SimpleChannelPool or FixedChannelPool constructor. Modifications: Redundant release method that takes offerHealthyOnly removed from ChannelPool. offerHealthyOnly parameter added to constructor for FixedChannelPool and SimpleChannelPool. Result: SimpleChannelPool and FixedChannelPool are now take offerHealthyOnly as a constructor parameter. Default behavior is: offerHealthyOnly=true. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:84: line made to be no longer then 120 characters. SimpleChannelPool.java:237: extra white space line removed. Result: Code satisfies checkstyle requirements. Tests do not need to be too copled to the code. Exception message should not be validated Motivation: We don't need our tests to be too coupled to the code. Exception type validation in tests is just good enough. Modifications: Exception validation message removed from SimpleChannelPoolTest.testUnhealthyChannelIsNotOffered() test. Result: The SimpleChannelPoolTest test is less coupled to the code now. Stack trace set to empty for UNHEALTHY_NON_OFFERED_TO_POOL. Motivation: We don't need stack trace for UNHEALTHY_NON_OFFERED_TO_POOL. Modifications: Added UNHEALTHY_NON_OFFERED_TO_POOL.setStackTrace(EmptyArrays.EMPTY_STACK_TRACE) to static init block. Result: UNHEALTHY_NON_OFFERED_TO_POOL's stack trace set to empty. Minor code re-factorings. Motivation: For better code readability we need to apply several minor code re-factorings. Modifications: javadocs true -> {@code true} offerHealthyOnly variable name changed to releaseHeathCheck <p/> -> <p> in javadocs offerHealthyOnly removed from doReleaseChannel as it not needed there. Result: Code quality is improved. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:87: line made to be no longer then 120 characters. Result: Code satisfies checkstyle requirements. Pull request needs to contain only necessary changes Motivation: The pull request should not contain unnecessary changes that are not needed as part of required functionality of pull request. Modifications: private void doReleaseChannel(final Channel channel, final Promise<Void> promise) - > private void doReleaseChannel(Channel channel, Promise<Void> promise) Result: Pull request contains less unnecessary modifications.
Fixed by #4078 |
@Scottmitch Please make sure not to introduce a build breakage when you cherry-pick into 4.0 and 4.1 |
@trustin - I'm sorry. Will be more careful in the future. Thanks for cleaning up after my mess. |
@Scottmitch No worries. I make the same mistake over and over. :-) |
Motivation: When releasing unhealthy channel back to a pool we don't have to offer it since on acquire it will be discarded anyways. Also checking healthiness at release is a good idea so we don't end up having tons of unhealthy channels in the pool(unless they became unhealthy after being offered) Modifications: private SimpleChannelPool.offerIfHealthy() method added that is called from SimpleChannelPool.doReleaseChannel(). SimpleChannelPool.offerIfHealthy() offers channel back to pool only if channel is healthy. Otherwise it throws setFailure exception to the promise. Result: The pool is now much cleaner and not spammed with unhealthy channels. Added ability to choose if channel health has to be validated on release by passing boolean flag. Motivation: Depending on performance preferences and individual use cases sometimes we would like to be able force health check of a channel at release time and do not offer it back to the pool. Other times we would want to just release channel and offer it back to the pool and check health only when we try to acquire that channel from the pool. See more details here: netty#4077 (comment) Modifications: Future<Void> release(Channel channel, Promise<Void> promise, boolean offerHealthyOnly); The offerHealthyOnly boolean flag allows developers to choose whether to do channel validation before offering it back to pool or not. Appropriate modifications made to hierarchy of implementations of ChannelPool. offerHealthyOnly=true will force channel health to be checked before offering back to pool. offerHealthyOnly=false will ignore channel health check and will just try just offer it back to the pool offerHealthyOnly=true by default. Result: Channel health check before offer back to pool is controlled by a flag now. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:279 line split to be less then 120 characters. SimpleChannelPool.java:280:31 space added after '{' SimpleChannelPool.java:282:17 space added after '{' SimpleChannelPoolTest.java:198 - extra white space line removed. Result: Code satisfies checkstyle requirements. offerHealthyOnly is passed as a constructor parameter now. Motivation: Instead of passing offerHealthyOnly as a method parameter it is better to pass it in as SimpleChannelPool or FixedChannelPool constructor. Modifications: Redundant release method that takes offerHealthyOnly removed from ChannelPool. offerHealthyOnly parameter added to constructor for FixedChannelPool and SimpleChannelPool. Result: SimpleChannelPool and FixedChannelPool are now take offerHealthyOnly as a constructor parameter. Default behavior is: offerHealthyOnly=true. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:84: line made to be no longer then 120 characters. SimpleChannelPool.java:237: extra white space line removed. Result: Code satisfies checkstyle requirements. Tests do not need to be too copled to the code. Exception message should not be validated Motivation: We don't need our tests to be too coupled to the code. Exception type validation in tests is just good enough. Modifications: Exception validation message removed from SimpleChannelPoolTest.testUnhealthyChannelIsNotOffered() test. Result: The SimpleChannelPoolTest test is less coupled to the code now. Stack trace set to empty for UNHEALTHY_NON_OFFERED_TO_POOL. Motivation: We don't need stack trace for UNHEALTHY_NON_OFFERED_TO_POOL. Modifications: Added UNHEALTHY_NON_OFFERED_TO_POOL.setStackTrace(EmptyArrays.EMPTY_STACK_TRACE) to static init block. Result: UNHEALTHY_NON_OFFERED_TO_POOL's stack trace set to empty. Minor code re-factorings. Motivation: For better code readability we need to apply several minor code re-factorings. Modifications: javadocs true -> {@code true} offerHealthyOnly variable name changed to releaseHeathCheck <p/> -> <p> in javadocs offerHealthyOnly removed from doReleaseChannel as it not needed there. Result: Code quality is improved. Code changed to satisfy checkstyle requirements. Motivation: Code needs to satisfy checkstyle requirements. Modifications: SimpleChannelPool.java:87: line made to be no longer then 120 characters. Result: Code satisfies checkstyle requirements. Pull request needs to contain only necessary changes Motivation: The pull request should not contain unnecessary changes that are not needed as part of required functionality of pull request. Modifications: private void doReleaseChannel(final Channel channel, final Promise<Void> promise) - > private void doReleaseChannel(Channel channel, Promise<Void> promise) Result: Pull request contains less unnecessary modifications.
Currently it is required to release channel to the channel pool for
FixedChannelPool
.Failure to release a channel may cause that
FixedChannelPool.acquiredChannelCount
becomes equal or bigger thenFixedChannelPool.maxConnections
. So basically it means that we have to release back all the channels even those ones that are not healthy, and when channel is released it simply put back into aCimpleChannelPool.deque
regardless if it's healthy or not.Now, on acquire SimpelChannelPool will check if channel is healthy before returning it. If it turns out to be unhealthy then it will discard it and poll the next one from the pool(if there any left in pool) and will continue doing so until it gets healthy channel or until the
deque
is empty(in which case it will just create a new channel). Though this sort of lazy-discard approach works, I think there is a room for improvement, such as we can filter out unhealthy channels at the time when we release them.Here is example where such improvement may help:
It also make sense to filter our unhealthy channels on release because in most cases when someone releases a channel he normally doesn't care about release performance. The more critical in my opinion is acquire performance.
The text was updated successfully, but these errors were encountered: