MINOR: a small refactor for LogManage#shutdown #9680
Conversation
Homework
|
@APaMio Thanks for your patch. Could you revise the title? The “caee2” is a bit weird to me :) |
chia7712
changed the title
| Try(future.get) match { | ||
| case Success(_) => false | ||
| case Failure(e) => | ||
| warn(s"There was an error in one of the threads during LogManager shutdown: ${e.getCause}") | ||
| true | ||
| } | ||
| }.contains(true) | ||
| } |
There was a problem hiding this comment.
Oh, my bad. Will revert it with suitable test.
There was a problem hiding this comment.
@chia7712 @ijuma I think you were referring to #9596 ?
The test added in #9596 to verify this was LogManagerTest.testHandlingExceptionsDuringShutdown (link). I agree that the test wasn't effective enough to catch the problem. The issue was that fundamentally this area of the code was a little bit difficult to test due to the asynchronous behavior.
So for example, although @chia7712 has a fix in #9728, I still think it is not easy to write a test that reliably fails if the bug is introduced again.
Below is my explanation on why it is difficult to improve the existing testHandlingExceptionsDuringShutdown to catch such issues. Let us imagine that the test setup introduced multiple logs for each log dir, and setup an expectation (in the test code) that if one of the logs failed during close(), then, the other logs under the same log directory should still be closed by the time LogManager.shutdown() returns. Such a kind of test expectation would have caught the problem introduced in this PR. Now, writing that kind of a test was not easy, because, even if one of the logs failed to close, the other futures can still be completed by the time LogManager.shutdown() returns (there is no strong need to call future.get for the future to pass). Thus, the test can pass despite the bug.
One potential solution (a bigger change) is to see if we can inject a different thread pool executor that only during test executes the threads lazily i.e. it executes the submitted threads only when future.get is called, otherwise it will never execute them. If we succeed in doing this, then, the test can be improved such that unless future.get is called on all logs, some of the log close() will never happen and will fail the test in the presence of the bug introduced by this PR.
cc @junrao
LogManage Modify
@chia7712