Cleanup for FindBugs and PMD warnings in -simple and -jena #49
Conversation
| public static final ThreadGroup threadGroup = new ThreadGroup("Commons RDF parsers"); | ||
| private static final ExecutorService threadpool = Executors.newCachedThreadPool(r -> new Thread(threadGroup, r)); | ||
| public static final AtomicInteger threadCount = new AtomicInteger(); | ||
| private static Thread newThread(Runnable r) { |
There was a problem hiding this comment.
I disagree on this style change, the previous code was much cleaner. Why is a method-reference on 4 lines that is only used once better than a very small lambda?
There was a problem hiding this comment.
Because either FindBugs or PMD (can't remember) which called out ThreadGroup as not threadsafe, which is correct as far as I know. I am in no way wedded to this change and if we can guarantee thread-safety from outside the class (or simply document it as not threadsafe) I would be happy to pull this change out.
There was a problem hiding this comment.
A final ThreadGroup that is not thread safe.. The whole purpose of it is to group threads?
It is true that they should not be used for security purpose, but that is not why it is here. The ThreadGroup is mainly useful for debugging as people might see these "Commons RDF Parser" tree grouped together in the debugger (e.g. in Eclipse) rather than the generic no-name you get from the ExecutorService.
There was a problem hiding this comment.
That's why I put the name "Commons RDF Parser " back in. But like I said, I am fine with whatever- I was just tearing through PMD warnings at speed after the comment (I think to the board report) about there being a lot of them. Shall I take your comment as a direction to remove this change? Happy to...
There was a problem hiding this comment.
OK, so I see ThreadGroup is criticized by Joshua Block, but I don't think we would get many of these threads in the pool, so no big need to group them. However we don't need that counter of the threadCount, which also don't need to be public, they can just be called Commons RDF Parser without a number.
No description provided.