GJL commented on a change in pull request #10255: [FLINK-14859][runtime] Avoid
leaking unassigned slots
URL: https://github.com/apache/flink/pull/10255#discussion_r348425294
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/TestExecutionSlotAllocator.java
##########
@@ -126,4 +131,13 @@ public void cancel(final ExecutionVertexID
executionVertexId) {
public CompletableFuture<Void> stop() {
return CompletableFuture.completedFuture(null);
}
+
+ @Override
+ public void returnLogicalSlot(final LogicalSlot logicalSlot) {
+ returnedSlots.add(logicalSlot);
+ }
+
+ public List<LogicalSlot> getReturnedSlots() {
+ return new ArrayList<>(returnedSlots);
Review comment:
I don't think it's needed to rename the method because defensive copying in
accessors is a common pattern, e.g., see Effective Java by Josh Bloch Item 15
_"Minimize mutability"_
([summary](https://github.com/mgp/book-notes/blob/master/effective-java-2nd-edition.markdown#item-15-minimize-mutability)).
This is done to reduce shared state. Moreover, returning a list that reflects
the current state instead of being a snapshot can be unexpected to the client
as well – it can lead to concurrency bugs, which admittedly are not an issue
here.
Below there are a few examples from the Flink codebase where defensive
copying in accessors is employed:
https://github.com/apache/flink/blob/9706261d60045f882ce05a42f3bd255e47fa348e/flink-table/flink-table-common/src/main/java/org/apache/flink/table/api/TableSchema.java#L214
https://github.com/apache/flink/blob/1ce777f328e064d0b36627e68f3d0d1d174d5414/flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/gateway/local/ResultStore.java#L104
https://github.com/apache/flink/blob/9706261d60045f882ce05a42f3bd255e47fa348e/flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/StandaloneCompletedCheckpointStore.java#L81
----------------------------------------------------------------
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:
us...@infra.apache.org
With regards,
Apache Git Services
