Guus der Kinderen commented on DIRMINA-1078:

Thanks for the feedback guys! I initially had this as a separate {{Executor}}, 
exactly for the reasons that you mention. You can see this variant of the code 
(note that I fixed a bug later on - don't blindly use this version!)

There was a _lot_ of code duplication going on. I thought about creating an 
abstract base class from which this implementation, but also  
{{OrderedThreadPoolExecutor}} would extend - but that proved to much of a 

With the patch that I introduced, I believe I was able to add the new 
functionality to the existing implementation of {{OrderedThreadPoolExecutor}}, 
as a completely _optional_, _not enabled by default_ behavior, that introduces 
next to _no behavioral changes_ if unused. Given this, I would personally 
prefer my addition to remain in {{OrderedThreadPoolExecutor}} instead of 
introducing a different {{ThreadPool}} implementation, but that's not a hill 
I'm willing to die on.

[~johnnyv], the {{FIFOEntry}} implementation is used as a tie-breaker, in case 
the {{Comparator}} that is provided does not distinguish between the sessions. 
It arguably can be left out, but I'd prefer to keep it in: It allows one to use 
relatively simple {{Comparator}} implementations, while still be ensured of 
defined behavior for those elements that are not covered in the {{Comparator}}. 
Again, not a hill I'm willing to die on though.

[~elecharny], if you're willing to transform my patch into a separate 
{{Executor}} (in case my arguments above were not compelling), please, be my 
guest. If anything, it'd save me time, and having a second set of eyes looking 
at this code will not hurt either.

> OrderedThreadPoolExecutor should allow sessions to be prioritized
> -----------------------------------------------------------------
>                 Key: DIRMINA-1078
>                 URL: https://issues.apache.org/jira/browse/DIRMINA-1078
>             Project: MINA
>          Issue Type: New Feature
>          Components: Core
>            Reporter: Guus der Kinderen
>            Priority: Minor
>         Attachments: 20180216.patch
> The functionality provided in {{OrderedThreadPoolExecutor}} should be 
> augmented to, optionally, allow for assignment of priority to certain 
> sessions over others.
> We've introduced this functionality after observing issues in a deployment 
> where system resources where being starved by the sheer amount of sessions 
> that attempted to perform TLS. Without the class introduced by this commit, 
> events for any session could eventually fail (time-out), causing the session 
> to fail. If that session happened to be a session that had already 
> established TLS, the resources that were spent on establishing TLS are 
> wasted. The negative effect is amplified by the fact that a typical client in 
> such a situation would attempt
>  to reconnect, which further adds to the load of the system already being 
> starved.
> With the modifications introduced by the patch provided in this issue, 
> priority can be given to sessions that have already established TLS. This 
> dramatically reduces the issue described above, as the first sessions to fail 
> will be those that are still negotiating TLS. Using a specialized 
> {{Comparator}}, one can even prioritize between these, causing sessions for 
> which least effort has performed to fail before sessions that are more likely 
> to near TLS completion.
> The patch intends to add this feature as optional functionality to the 
> existing implementation, with little side effects to the existing, default 
> behavior.
> The implementation provided here was initially based on a copy of the 
> implementation of {{OrderedThreadPoolExecutor}} that introduced a 
> considerable amount of code duplication. For illustrative purposes, the line 
> of commits leading from that initial commit to the patch attached to this 
> JIRA issue can be found at 
> [https://github.com/guusdk/mina/commit/c0a421cf445696fbfd4d5b10d650d7c71d8faab7]
>  and later commits.

This message was sent by Atlassian JIRA

Reply via email to