[ https://issues.apache.org/jira/browse/DIRMINA-1078?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16368890#comment-16368890 ]
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 here: https://github.com/guusdk/mina/commit/c0a421cf445696fbfd4d5b10d650d7c71d8faab7 (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 hassle. 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 (v7.6.3#76005)