zentol commented on a change in pull request #10551: [FLINK-13662][kinesis]
Relax timing requirements
URL: https://github.com/apache/flink/pull/10551#discussion_r360404427
##########
File path:
flink-connectors/flink-connector-kinesis/src/test/java/org/apache/flink/streaming/connectors/kinesis/FlinkKinesisProducerTest.java
##########
@@ -325,16 +325,20 @@ public void go() throws Exception {
};
moreElementsThread.start();
- moreElementsThread.trySync(deadline.timeLeftIfAny().toMillis());
assertTrue("Producer should still block, but doesn't",
moreElementsThread.isAlive());
// consume msg-2 from the queue, leaving msg-3 in the queue and
msg-4 blocked
+ while (producer.getPendingRecordFutures().size() < 2) {
Review comment:
I think that only makes sense when looking at the diff and treating this
change as a drop-in replacement.
What actually happened was that I removed the syncs because they are
pointless; they don't provide any guarantees that anything of interest has
happened. In fact the same could be said the for isAlive() checks; after all
the thread can die just after the check. But for the rare case where they do
find an issue they provide a more interesting error message than a
TimeoutException.
After the removal the test started failing since it relies on elements being
processed in the background. Hence the loops were added, immediately before the
code that relies on some processing to have happened
(`getPendingRecordFutures().get(2)`).
Putting the loop before the isAlive() calls renders these calls basically
pointless since it doesn't make sense for the thread to die _after having
finished what it's supposed to do_, which the loop verifies.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
