pnowojski commented on a change in pull request #14348:
URL: https://github.com/apache/flink/pull/14348#discussion_r539334528
##########
File path:
flink-tests/src/test/java/org/apache/flink/test/checkpointing/UnalignedCheckpointITCase.java
##########
@@ -321,7 +321,7 @@ public void invoke(Long value, Context context) throws
Exception {
if (backpressure) {
// induce backpressure until enough checkpoints
have been written
if (random.nextInt(100) == 42) {
- Thread.sleep(0, 100_000);
+ Thread.sleep(100);
Review comment:
With modified `Thread.sleep()` I presume CPU usage went down, right?
But what about test duration? Has it increased? If so maybe
```
if (random.nextInt(100) == 42) {
Thread.sleep(1);
}
```
is better?
Note that previous `Thread.sleep(0, 100_000);` was basically an equivalent
of `Thread.sleep(0);` which in turn was burning 100% cpu. In other words
```
if (random() % 100 == 0) {
Thread.sleep(1);
}
```
works usually much better and more like you would be expecting compared to
```
Thread.sleep(0, 100_000);
```
which is for the most purposes just broken and not sleeping at all.
##########
File path:
flink-tests/src/test/java/org/apache/flink/test/checkpointing/UnalignedCheckpointITCase.java
##########
@@ -321,7 +321,7 @@ public void invoke(Long value, Context context) throws
Exception {
if (backpressure) {
// induce backpressure until enough checkpoints
have been written
if (random.nextInt(100) == 42) {
- Thread.sleep(0, 100_000);
+ Thread.sleep(100);
Review comment:
Simplify to just `Thread.sleep(1)`?
##########
File path:
flink-tests/src/test/java/org/apache/flink/test/checkpointing/UnalignedCheckpointTestBase.java
##########
@@ -103,11 +101,6 @@
@Rule
public final TemporaryFolder temp = new TemporaryFolder();
- @Rule
- public final Timeout timeout = Timeout.builder()
- .withTimeout(300, TimeUnit.SECONDS)
- .build();
Review comment:
Drawback of removing such timeout is, that if there are multiple
deadlocks in single azure build, test level timeout would show all of them.
Azure level would show just the first one. Secondly won't the Azure level
timeout would take longer to kick in?
I'm not saying I'm strictly against removing it. But I'm just pointing this
out. Having test level timeout is also sometimes annoying during debugging, so
all in all I'm +/- 0 for this change.
----------------------------------------------------------------
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]
