You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/07/09 04:24:28 UTC
[GitHub] [flink] zhuzhurk commented on a diff in pull request #20153: [FLINK-28144][runtime] Let JobMaster support blocklist.
zhuzhurk commented on code in PR #20153:
URL: https://github.com/apache/flink/pull/20153#discussion_r917217609
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1384,8 +1374,9 @@ public void testRequestPartitionState() throws Exception {
fail("Expected failure.");
Review Comment:
Let's replace the try-catch block with a `assertThatThrownBy`
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -959,20 +951,20 @@ public void testReconnectionAfterDisconnect() throws Exception {
// wait for first registration attempt
final JobMasterId firstRegistrationAttempt = registrationsQueue.take();
- assertThat(firstRegistrationAttempt, equalTo(jobMasterId));
+ assertThat(firstRegistrationAttempt).isEqualTo(jobMasterId);
- assertThat(registrationsQueue.isEmpty(), is(true));
+ assertThat(registrationsQueue.isEmpty()).isTrue();
Review Comment:
can be `assertThat(registrationsQueue).isEmpty();`
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1384,8 +1374,9 @@ public void testRequestPartitionState() throws Exception {
fail("Expected failure.");
} catch (ExecutionException e) {
assertThat(
- ExceptionUtils.findThrowable(e, IllegalArgumentException.class).isPresent(),
- is(true));
+ ExceptionUtils.findThrowable(e, IllegalArgumentException.class)
Review Comment:
Would `hasRootCauseInstanceOf(IllegalArgumentException.class)` work here?
If not, we can still simplify it to be `assertThat(ExceptionUtils.findThrowable(e, IllegalArgumentException.class)).isPresent()`.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1462,19 +1455,19 @@ public void testTriggerSavepointTimeout() throws Exception {
try {
savepointFutureLowTimeout.get(testingTimeout.getSize(), testingTimeout.getUnit());
- fail();
+ fail("Expected TimeoutException");
} catch (final ExecutionException e) {
final Throwable cause = ExceptionUtils.stripExecutionException(e);
- assertThat(cause, instanceOf(TimeoutException.class));
+ assertThat(cause).isInstanceOf(TimeoutException.class);
}
- assertThat(savepointFutureHighTimeout.isDone(), is(equalTo(false)));
+ assertThat(savepointFutureHighTimeout.isDone()).isFalse();
Review Comment:
can be `assertThat(savepointFutureHighTimeout).isNotDone();`
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1414,9 +1406,10 @@ public void testRequestPartitionState() throws Exception {
fail("Expected failure.");
} catch (ExecutionException e) {
assertThat(
- ExceptionUtils.findThrowable(e, PartitionProducerDisposedException.class)
- .isPresent(),
- is(true));
Review Comment:
See above comment.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1398,8 +1389,9 @@ public void testRequestPartitionState() throws Exception {
fail("Expected failure.");
} catch (ExecutionException e) {
assertThat(
Review Comment:
See above 2 comments.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1593,18 +1585,18 @@ public void testTaskExecutorNotReleasedOnFailedAllocationIfPartitionIsAllocated(
// we should free the slot, but not disconnect from the TaskExecutor as we still have an
// allocated partition
- assertThat(freedSlotFuture.get(), equalTo(allocationId));
+ assertThat(freedSlotFuture.get()).isEqualTo(allocationId);
// trigger some request to guarantee ensure the slotAllocationFailure processing if
// complete
jobMasterGateway.requestJobStatus(Time.seconds(5)).get();
- assertThat(disconnectTaskExecutorFuture.isDone(), is(false));
+ assertThat(disconnectTaskExecutorFuture.isDone()).isFalse();
Review Comment:
can be `assertThat(disconnectTaskExecutorFuture).isNotDone();`
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java:
##########
@@ -1462,19 +1455,19 @@ public void testTriggerSavepointTimeout() throws Exception {
try {
savepointFutureLowTimeout.get(testingTimeout.getSize(), testingTimeout.getUnit());
- fail();
+ fail("Expected TimeoutException");
Review Comment:
better to use `assertThatThrownBy`
##########
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/slotpool/DeclarativeSlotPoolServiceTest.java:
##########
@@ -230,14 +228,13 @@ public void testCreateAllocatedSlotReport() throws Exception {
declarativeSlotPoolService.createAllocatedSlotReport(
taskManagerLocation2.getResourceID());
- assertThat(
- allocatedSlotReport.getAllocatedSlotInfos(),
- contains(matchesWithSlotContext(simpleSlotContext2)));
+ assertThat(allocatedSlotReport.getAllocatedSlotInfos())
+ .is(matching(contains(matchesWithSlotContext(simpleSlotContext2))));
Review Comment:
This matcher is simpler so I prefer to drop it and rewrite the verification to be:
```
assertThat(allocatedSlotReport.getAllocatedSlotInfos())
.allMatches(
context ->
context.getAllocationId()
.equals(simpleSlotContext2.getAllocationId())
&& context.getSlotIndex()
== simpleSlotContext2.getPhysicalSlotNumber());
```
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org