Pool improvements #77
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The use of dameon threads will help avoid issues where mariaDb is holding up shutdown of applications. In addition both pool implementations now default to the same "MariaDbThreadFactory" to reduce code duplication.
Instead of using timers which create threads on demand, this leverages the existing "DynamicSizedScheduler" concept to allow a pool to reuse possible threads. Thus avoiding the need to create possibly lots of threads if there is a server side issue requiring lots of queries to timeout. Using this dynamic pool we can potentially reusue threads.
For the most part this ArrayDeque was locked well, but there was a couple places where it could be accessed without being locked. This solves those few conditions, and adds comments along the breadcomb trail to help guard against those assumptions from being violated later on.
|
Well done |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It is probably easier to digest through the commit's, but I figured I would include it all in a single PR. Feel free to make requests for individual commit adjustments (or removals).
46c4166
This commit is address an issue @rusher pointed me to with tomcat struggling to shutdown. His recommendation was to track the connections and unscheduled the tasks as needed, and reschedule if we end up creating new connections.
I took a different angle to this. We have not run into these issues, and I suspect it's because we are using https://github.com/threadly/threadly/ under the hood which defaults to daemon threads. I am not aware of anything that we would need to hold the app shutdown up for, so it seems like transitioning these threads to daemon is has the least performance impact, and is the easiest option.
efc6405
This one was a slight pet peeve of mine, but @rusher said he would be open to seeing a possible PR solution. I dislike Timer's because of how they create threads all over the place, and much more prefer pools. In this implementation we use the existing
DynamicSizedSchedulerto be able to pool these requests.I considered a trivial implementation where we just increase the pool size in
setTimerTask, then decreased it instopTimeoutTaskif it's set. But I feel like even though it's a bit more code, it's worth it to support this natively in theDynamicSizedScheduler. This seems the safest, and likely could get reused later on. In addition by supporting it natively we will be able to get even greater benefit when using our threadly schedulers (it will be super clean, though it would be even cleaner if we just removed all of this and used a threadly "Watchdog" instead, but I understand not wanting to add the dependency).The final commit ca0acb2 addresses an issue I brought up in PR where this structure is not guaranteed to be locked, and may be accessed concurrently. I solved those couple conditions and added a few comments where those locked assumptions where being made.
As always, all these changes are submitted under the BSD-new license.