MINOR: fix checkpoint write failure warning log#6008
MINOR: fix checkpoint write failure warning log#6008guozhangwang merged 4 commits intoapache:trunkfrom vvcephei:fix-checkpoint-warning
Conversation
|
@guozhangwang @bbejeck , do you mind taking a look at this when you get the chance? |
|
checkstyle failure... I'll look into it tomorrow. |
| checkpoint.write(this.checkpointableOffsets); | ||
| } catch (final IOException e) { | ||
| log.warn("Failed to write offset checkpoint file to {}: {}", checkpoint, e); | ||
| if (log.isWarnEnabled()) { |
There was a problem hiding this comment.
nit: we don't need log. isWarnEnabled() if we use
log.warn("Failed to write offset checkpoint file to [{}]", checkpoint, e); I pulled down the branch and tested this out.
The issue with the current log statement in trunk is an extra pair of {} for the exception parameter but that is handled automatically.
| final LogCaptureAppender.Event lastEvent = messages.get(messages.size() - 1); | ||
|
|
||
| assertThat(lastEvent.getLevel(), is("WARN")); | ||
| assertThat(lastEvent.getMessage(), startsWith("process-state-manager-test Failed to write offset checkpoint file to [/tmp/")); |
There was a problem hiding this comment.
This test fails on the mac on this line as the path is not /tmp but starts with /var/folders... by changing the assertion to startsWith("process-state-manager-test Failed to write offset checkpoint file to [ then the test passes
There was a problem hiding this comment.
This makes perfect sense. I should have predicted that the "temporary file" directory is different on every OS.
| final String[] throwableStrRep = event.getThrowableStrRep(); | ||
| final Optional<String> throwableString; | ||
| if (throwableStrRep == null) { | ||
| throwableString = Optional.empty(); |
There was a problem hiding this comment.
nit: maybe we should set a default value instead? If this is empty the test will fail from NoSuchElementException. I know you are aware of this, I'm just thinking about in the future about others using this class and having to test for isPresent() vs. get()
There was a problem hiding this comment.
This is exactly what I was going for. The type (Optional) documents that you have to check isPresent before get. It's just as true if we make the field nullable, but then it's not well documented, and therefore more vulnerable to NPEs.
In my test, I deliberately don't check isPresent because I expect it to be present. If it's not, we'll get a NoSuchElementException, which contains the exact same information as if we had an extra isPresent check that then throws an AssertionException(Expected a throwable).
There was a problem hiding this comment.
If you think it's better to add the isPresent assertion, I can do it; I just tend not to do such checks in tests, for the reason above.
There was a problem hiding this comment.
No, your arguments make sense, I'd leave it as is.
|
Thanks for the review, @bbejeck ! |
|
Unrelated java 8 failure: Retest this, please. |
|
Unrelated java 11 failure: Retest this, please. |
We saw a log statement in which the cause of the failure to write a checkpoint was not properly logged. This change logs the exception properly and also verifies the log message. Reviewers: Bill Bejeck <bill@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
3 participants
We saw a log statement in which the cause of the failure to write a checkpoint was not properly logged.
This change logs the exception properly and also verifies the log message.
Committer Checklist (excluded from commit message)