[GitHub] [flink] tillrohrmann commented on a change in pull request #10362: [FLINK-14792][coordination] Implement TE cluster partition release

[GitBox <gi...@apache.org>]

tillrohrmann commented on a change in pull request #10362: [FLINK-14792][coordination] Implement TE cluster partition release
URL: https://github.com/apache/flink/pull/10362#discussion_r353899948
 
 

 ##########
 File path: flink-runtime/src/test/java/org/apache/flink/runtime/taskexecutor/TaskExecutorPartitionLifecycleTest.java
 ##########
 @@ -249,6 +250,22 @@ public void testPartitionPromotion() throws Exception {
 		);
 	}
 
+	@Test
+	public void testClusterPartitionRelease() throws Exception {
+		testPartitionRelease(
+			(jobId, partitionId, taskExecutor, taskExecutorGateway, partitionTracker) -> {
+				final CompletableFuture<Collection<IntermediateDataSetID>> releasePartitionsFuture = new CompletableFuture<>();
+				runInTaskExecutorThreadAndWait(taskExecutor, () -> partitionTracker.setReleaseClusterPartitionsConsumer(releasePartitionsFuture::complete));
+
+				final IntermediateDataSetID dataSetId = new IntermediateDataSetID();
+
+				taskExecutorGateway.releaseClusterPartitions(Collections.singleton(dataSetId));
+
+				assertThat(releasePartitionsFuture.get(), hasItems(dataSetId));
+			}
+		);
+	}
 
 Review comment:
   I'm not entirely sure that this test is really necessary. What we are effectively testing here is that the `TaskExecutor` forwards the `releaseClusterPartitions` to the `partitionTracker`. For that to test, we do quite some extra work in `testPartitionRelease`. I think if we only want to test the call forwarding, then we don't have to do the extra work done by `testPartitionRelease`. If we want to test the integration of the `TaskExecutor` and the `TaskExecutorPartitionTracker`, then we might consider writing an integration test which uses the proper implementation. Of course, then we need to choose a different way to verify the correct behaviour (different level of mock components).

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services