zentol 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_r387670966
##########
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:
While it does do a fair amount of setup work it does verify the expected
behavior for the _expected circumstances_; a partition release after a task has
finished.
The whole point of `testPartitionRelease` is to create a "realistic"
environment; remove any part of it and we are working in a theoretical scenario
that may (now or at some point) no longer represent reality.
We could streamline the test a bit such that we test all things at once (so
we only pay the setup cost once), but that would be out-of-scope of this PR.
At the same time the current test(s) have the usual problem of our
mocking-like approach; they check that something was called, but other
interactions are fully ignored. Take `testPartitionPromotion` as an example;
for all we know the TE may (wrongly) release all partitions after the
promotion, and the test wouldn't cover that.
I would prefer if the tests were to use the actual implementation, but I
don't know of a good way to wait for the TM to have fully processed a request.
I remember that we were looking to a solution for this some time ago, but don't
remember what the conclusion was.
----------------------------------------------------------------
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
