Github user tillrohrmann commented on a diff in the pull request:

    https://github.com/apache/flink/pull/4937#discussion_r148601504
  
    --- Diff: 
flink-runtime/src/main/java/org/apache/flink/runtime/instance/SlotPool.java ---
    @@ -645,6 +668,21 @@ AllocatedSlots getAllocatedSlots() {
                return allocatedSlots;
        }
     
    +   @VisibleForTesting
    +   AvailableSlots getAvailableSlots() {
    +           return availableSlots;
    +   }
    +
    +   @VisibleForTesting
    +   int getNumOfWaitingForResourceRequests() {
    +           return waitingForResourceManager.size();
    +   }
    +
    +   @VisibleForTesting
    +   int getNumOfPendingRequests() {
    +           return pendingRequests.size();
    +   }
    --- End diff --
    
    I think we should not make internal state easily accessible because it will 
usually be modified by the main thread. Also when checking a certain 
interleaving you might be falsely entrapped that you can do something like
    ```
    slotPool.asyncAddPendingRequest()
    slotPool.getNumOfPendingRequests() // this returns +1 pending requests
    ```
    
    This is might work but sometimes it also does not work because the 
concurrent operation has not been completed. I would like to make concurrent 
operations explicit by, for example, returning a `CompletableFuture<Integer> 
getNumberOfPendingRequests` if at all.


---

Reply via email to