Small logging + logic clean up of concurrent collection #210
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
* Remove a duplicate * Change misleading entries * Clarify some entries
Now that we validate that the configured number of threads is strictly positive.
We need to compare the current number of active tasks against the max number of threads, not the current (and likely below max) number of threads. Added a test assertion.
truthbk
reviewed
Feb 26, 2019
|
Updated PR description with our inline discussion. Merging! |
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.
What this PR does
See individual commit descriptions. Overall:
CachedThreadPoolcreation, since the config code doesn't allow negative values for the number of threads at the moment. A future PR (for0.27.0) will allow negative values.tpe.getPoolSize()is0, so the app would consider the pool as not ready and print out warnings + recreate the thread pool as long the first collection isn't actually kicked off.Notes
App.processFixedStatusdoesn't log anything at the moment, not sure whether logging from there would potentially spam a lot of logs, would appreciate suggestions on that.Overall, 2180909 requires particular attention as it changes actual logic, but the other changes shouldn't require any QA.