Conversation
k-rus
left a comment
k-rus
left a comment
There was a problem hiding this comment.
LGTM
I have few nit comments. I also see that the PR's quality is not accepted by SolarCloud. So I guess I will need to look again when it is satisfied.
| } | ||
|
|
||
| if (err != null) | ||
| logger.error("Got error when removing shutdown hook(s): {}", err.getMessage(), err); |
There was a problem hiding this comment.
Should it be?:
| logger.error("Got error when removing shutdown hook(s): {}", err.getMessage(), err); | |
| logger.error("Got error(s) when removing shutdown hook(s): {}", err.getMessage(), err); |
There was a problem hiding this comment.
Is it correct to log the error if
killJVMwas called to be quiet? To me it sounds as violation of the interface description:@param quiet - whether the error should be logged verbosely
In this case, I think it's ok.
The quiet flag controls the verbose printing of a stack trace to syserr and log file about the error that triggered the shutdown.
However, this log message is about a secondary error while removing shutdown hooks and may be important diagnostic/debug information.
In other words, the quiet flag means shutdown without verbose reporting on the error that triggered the shutdown, but if there is a subsequent error removing shutdown hooks, those are still logged (not quite as verbosely since it goes to the log file only).
Well, in the end I guess it's subjective but imo the current arrangement is likely intended and seems ok to me.
There was a problem hiding this comment.
I agree. I should be more thoughtful here during review :)
BTW, when quiet is false the print of error contains unnecessary extra space between the sentences in this line of the implementation.
There was a problem hiding this comment.
No worries, it was a good discussion to have :)
Sorry, I didn't notice the test coverage earlier; I have applied some updates and tried to improve test coverage. |
|
Kudos, SonarCloud Quality Gate passed! |
…oks (#271) (cherry picked from commit c6cd4ee) (cherry picked from commit fb5c241) (cherry picked from commit f7f8e0a) (cherry picked from commit 0ff7339) (cherry picked from commit e7ca94c) STAR-891 Fix conflict in DiskFailurePolicyTest with CASSANDRA-18294 (cherry picked from commit 07cd5c9) (Rebase of commit 3ac50e1)
…oks (#271) (cherry picked from commit c6cd4ee) (cherry picked from commit fb5c241) (cherry picked from commit f7f8e0a) (cherry picked from commit 0ff7339) (cherry picked from commit e7ca94c) STAR-891 Fix conflict in DiskFailurePolicyTest with CASSANDRA-18294 (cherry picked from commit 07cd5c9) (Rebase of commit 3ac50e1)
…oks (#271) (cherry picked from commit c6cd4ee) (cherry picked from commit fb5c241) (cherry picked from commit f7f8e0a) (cherry picked from commit 0ff7339) (cherry picked from commit e7ca94c) STAR-891 Fix conflict in DiskFailurePolicyTest with CASSANDRA-18294 (cherry picked from commit 07cd5c9) (Rebase of commit 3ac50e1)
…oks (#271) (cherry picked from commit c6cd4ee) (cherry picked from commit fb5c241) (cherry picked from commit f7f8e0a) (cherry picked from commit 0ff7339) (cherry picked from commit e7ca94c) STAR-891 Fix conflict in DiskFailurePolicyTest with CASSANDRA-18294 (cherry picked from commit 07cd5c9) (Rebase of commit 3ac50e1)
…oks (#271) (cherry picked from commit c6cd4ee) (cherry picked from commit fb5c241) (cherry picked from commit f7f8e0a) (cherry picked from commit 0ff7339) (cherry picked from commit e7ca94c) STAR-891 Fix conflict in DiskFailurePolicyTest with CASSANDRA-18294 (cherry picked from commit 07cd5c9) (Rebase of commit 3ac50e1)
3 participants
No description provided.