ferenc-csaky commented on code in PR #24429:
URL: https://github.com/apache/flink/pull/24429#discussion_r1514835462
##########
flink-streaming-java/src/test/java/org/apache/flink/streaming/api/functions/sink/SocketClientSinkTest.java:
##########
@@ -178,36 +142,31 @@ public void run() {
simpleSink.open(DefaultOpenContext.INSTANCE);
// wait socket server to close
- serverRunner.join();
- if (error.get() != null) {
- Throwable t = error.get();
- t.printStackTrace();
- fail("Error in server thread: " + t.getMessage());
- }
-
- try {
- // socket should be closed, so this should trigger a re-try
- // need two messages here: send a fin to cancel the client
state:FIN_WAIT_2 while
- // the server is CLOSE_WAIT
- while (true) { // we have to do this more often as the server
side closed is not
- // guaranteed to be noticed immediately
- simpleSink.invoke(TEST_MESSAGE + '\n',
SinkContextUtil.forTimestamp(0));
- }
- } catch (IOException e) {
- // check whether throw a exception that reconnect failed.
- assertTrue("Wrong exception",
e.getMessage().contains(EXCEPTION_MESSGAE));
- } catch (Exception e) {
- fail("wrong exception: " + e.getClass().getName() + " - " +
e.getMessage());
- }
-
- assertEquals(0, simpleSink.getCurrentNumberOfRetries());
- } finally {
- IOUtils.closeQuietly(server);
Review Comment:
If there are any chance `ServerSocket` cannot be closed, try-with-resources
will throw that exception, so I think it might introduce some flakiness. I
wonder if it would worth it to keep the original try/finally with the quiet
close. WDYT?
##########
flink-streaming-java/src/test/java/org/apache/flink/streaming/api/functions/sink/TwoPhaseCommitSinkFunctionTest.java:
##########
@@ -66,21 +62,21 @@ public class TwoPhaseCommitSinkFunctionTest {
private SettableClock clock;
- @Rule
- public final TestLoggerResource testLoggerResource =
- new TestLoggerResource(TwoPhaseCommitSinkFunction.class,
Level.WARN);
+ @RegisterExtension
+ private LoggerAuditingExtension testLoggerResource =
+ new LoggerAuditingExtension(TwoPhaseCommitSinkFunction.class,
Level.INFO);
Review Comment:
Does changing the log level from `WARN` to `INFO` makes any difference here?
##########
flink-streaming-java/src/test/java/org/apache/flink/streaming/api/graph/SlotAllocationTest.java:
##########
@@ -39,10 +37,10 @@
* sharing groups.
*/
@SuppressWarnings("serial")
-public class SlotAllocationTest extends TestLogger {
+class SlotAllocationTest {
@Test
- public void testTwoPipelines() {
+ void testTwoPipelines() {
Review Comment:
`dummyFilter` can be replaced with lambda here too. Maybe it could be a
`private` class level field, as it is duplicated in 3 test cases.
##########
flink-streaming-java/src/test/java/org/apache/flink/streaming/api/operators/collect/utils/AbstractTestCoordinationRequestHandler.java:
##########
Review Comment:
The ctor could be `protected`, or the ctor and the currently `protected`
fields could be pakcage-private.
##########
flink-streaming-java/src/test/java/org/apache/flink/streaming/api/operators/async/AsyncWaitOperatorTest.java:
##########
@@ -112,11 +105,12 @@
* <li>Snapshot state and restore state
* </ul>
*/
-public class AsyncWaitOperatorTest extends TestLogger {
+@Timeout(100)
Review Comment:
nit: Maybe add the `unit` in the annotation or a comment to make it more
explicit it is in seconds.
##########
flink-streaming-java/src/test/java/org/apache/flink/streaming/api/operators/async/queue/OrderedStreamElementQueueTest.java:
##########
@@ -67,8 +66,8 @@ public void testCompletionOrder() {
new StreamRecord<>(11, 1L),
new Watermark(2L),
new StreamRecord<>(13, 3L));
- Assert.assertEquals(expected, popCompleted(queue));
- Assert.assertEquals(0, queue.size());
- Assert.assertTrue(queue.isEmpty());
+ assertThat(popCompleted(queue)).isEqualTo(expected);
+ assertThat(queue.size()).isEqualTo(0L);
Review Comment:
`.isEqualTo(0L)` -> `.isZero()`
--
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]
