AMQ-9824 - Cleanup code in KahaDB classes #1567
Conversation
This cleanups the MessageDatabase and KahaDBStore classes. This commit includes the following: * Fixes the scope of several methods and types. For example, there were many cases where protected methods were referencing types that were package scope. * Simplified the code by replacing anonymous methods with lambdas * removed unused methods and parameters * removed unnecessary casts * cleaned up the use of generics where types could be inferred * Replaced the ReentrantReadWriteLock that was used for indexLock with ReentrantLock becuase only the write lock was ever being used (the page file doesn't support concurrent reads right now). This should provide a small performance/memory improvement and simplifies the code a bit. * removed unnecessary null initializations * cleaned up logging to remove string concatenation and instead use parameters * removed method overrides that are the same as the parent or just call the super method * removed unused checked exceptions from method's throws * marked inner classes as static when possible
|
Note that this PR includes the change from #1566 , once that is merged i'll merge main back into this branch (or force push) so it is gone from the diff Edit: That PR was merged and I merged main into this branch now. |
|
What is everyone's thoughts on back porting to older branches? The diff is large because of the cleanup but there should not be breaking changes, it's all just clean up and fixing issues. No public API or anything is effected so in terms of Semver it is fine. Back porting would be nice because if we don't then if we have to fix any future KahaDB bugs we would be more likely to run into some merge conflicts if trying to backport those and these changes are not there as well. There were some JDK 17 stuff that I didn't do such as switch to more modern switch statements and instanceof matching because i wasn't sure if we'd backport to 5.x or not. That stuff could be done if we are not. I think either way is probably fine, we probably wouldn't be doing a lot with KahaDB in older branches so may not be much of an issue I just figured I'd bring it up. |
|
I think back-porting is a good idea, just to have continuity a little longer if there are any security backport items. Perhaps we make 5.20.x that is JDK 17-based w/ javax.jms.*. Interesting about the indexLock was only ever using the write lock. |
| protected ExecutorService topicExecutor; | ||
| protected final List<Map<AsyncJobKey, StoreTask>> asyncQueueMaps = new LinkedList<>(); | ||
| protected final List<Map<AsyncJobKey, StoreTask>> asyncTopicMaps = new LinkedList<>(); | ||
| final List<Map<AsyncJobKey, StoreTask>> asyncQueueMaps = new LinkedList<>(); |
There was a problem hiding this comment.
What is the thought on narrowing the scope here? What if someone extended the class but it is not in the same package?
There was a problem hiding this comment.
All the scope changes were done because they were previously incorrect and my IDE flagged it. In this case, AsyncJobKey is package scope and not protected.
It would is impossible for someone to extend the class and use AsyncJobKey anyways, etc so changing the scope didn't cause problems. The two choices were to narrow that line or to expand the scope and my preference is always to narrow vs expand unless we need to. Generally speaking with KahaDB most things should likely just be package scope anyways.
|
+1 to backport so that future maintenance is easier. |
3 participants
This cleanups the MessageDatabase and KahaDBStore classes.
This commit includes the following: