HDDS-15114. Replace misconfigured ThreadPoolExecutor with Executors factory methods#10133
HDDS-15114. Replace misconfigured ThreadPoolExecutor with Executors factory methods#10133ptlrs wants to merge 5 commits into
Conversation
|
Thanks @ptlrs for updating the patch. Can we add atleast one integration test testing the change because now there can be multiple concurrent worker threads running for downloading the OM DB tar for multiple sst files. May be a test that verifies tasks submitted to |
…llection and improve related test coverage.
…add configurable thread pool size for DataNode metrics collection.
priyeshkaratha
left a comment
priyeshkaratha
left a comment
There was a problem hiding this comment.
Thanks @ptlrs for the patch. Changes LGTM
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @ptlrs for the patch. I think TestTarExtractor is a bit too heavy and possibly flaky for a test that verifies built-in functionality (thread pool). Can it be changed to just test the creation of the thread pool? (Mock Executors static methods and check that the right one is called.)
| if (extractionError.get() != null) { | ||
| throw new AssertionError("Tar extraction failed", extractionError.get()); |
There was a problem hiding this comment.
nit: why not assertNull?
| throw new AssertionError("Tar extraction failed", extractionError.get()); | ||
| } | ||
|
|
||
| assertEquals(poolSize, writeFileThreads.size(), |
There was a problem hiding this comment.
Will this be flaky, i.e. fail if the loop does not manage to capture all extractor threads?
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @ptlrs for updating the patch.
4 participants
What changes were proposed in this pull request?
Some threadpools are misconfigured. Their effective number of threads is one or they never reach their maximum number of threads.
This PR:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15114
How was this patch tested?
CI: https://github.com/ptlrs/ozone/actions/runs/24918410704