XComp commented on code in PR #24484:
URL: https://github.com/apache/flink/pull/24484#discussion_r1525984268
##########
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/logging/LoggerAuditingExtension.java:
##########
@@ -81,7 +81,9 @@ public void beforeEach(ExtensionContext context) throws
Exception {
new AbstractAppender("test-appender", null, null, false,
Property.EMPTY_ARRAY) {
@Override
public void append(LogEvent event) {
- loggingEvents.add(event.toImmutable());
+ if (event.toImmutable() != null) {
+ loggingEvents.add(event.toImmutable());
+ }
Review Comment:
It's not clear to me why this change is necessary. The `LogEvent` interface
doesn't indicate that `null` is a valid return value. :thinking: You might want
to add a comment here if we really need to check for `null` values here.
##########
flink-tests/src/test/java/org/apache/flink/test/misc/JobIDLoggingITCase.java:
##########
Review Comment:
Just to minor nitty general comments on JUnit5 testing:
* JUnit5 doesn't require for the test class to be public, anymore. This also
removes the necessity to add JavaDoc to please checkstyle.
* Same is true for the test methods
##########
flink-tests/src/test/java/org/apache/flink/test/misc/JobIDLoggingITCase.java:
##########
@@ -205,6 +268,8 @@ private static JobID runJob(ClusterClient<?> clusterClient)
throws Exception {
// wait for all tasks ready and then checkpoint
while (true) {
try {
+ clusterClient.triggerCheckpoint(jobId,
CheckpointType.DEFAULT).get();
+ // get checkpoint notification
Review Comment:
why do need a second checkpoint trigger to get the checkpoint notification?
because the 2nd checkpoint is waiting for the first one to finish? :thinking:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
