[GitHub] [flink] xintongsong commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default

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

xintongsong commented on a change in pull request #10921: [FLINK-15692][state backend] Enable limiting RocksDB memory consumption by default
URL: https://github.com/apache/flink/pull/10921#discussion_r369906968
 
 

 ##########
 File path: flink-runtime/src/test/java/org/apache/flink/runtime/state/StateBackendMigrationTestBase.java
 ##########
 @@ -71,12 +74,26 @@
 
 	protected abstract B getStateBackend() throws Exception;
 
+	protected abstract long getManagedMemorySize();
 
 Review comment:
   @StephanEwen
   I find it difficult to follow the entire conversation, but I can try to explain what's the problem of having on-heap managed memory for RocksDB state backend test cases: 
   - RocksDB state backend (or to be specific `RocksDBOperationUtils#allocateSharedCachesIfConfigured`) calls `MemoryManager#getSharedMemoryResourceForManagedMemory` for reserving and allocating managed memory, with 1.0 as the `fractionToInitializeWith`.
   - In `getSharedMemoryResourceForManagedMemory`, we first compute the memory size with the fraction 1.0, then reserve that amount of memory.
   - While `computeMemorySize` returns memory size including both on-heap and off-heap, we reserve only off-heap memory.
   - If the memory manager contains both on-heap and off-heap memory, no matter eagerly allocated/reserved or not, we will try to reserve more off-heap memory than it has.
   - E.g., memory manager contains 50mb on-heap budget and 100mb off-heap budget, in `getSharedMemoryResourceForManagedMemory` we will first compute the memory size as 150mb, then try to reserve 150mb off-heap memory which is larger than the off-heap budget.
   
   Currently memory manager created from the testing util class `MemoryManagerBuilder` will always contain some on-heap memory, which is added in the constructor of `MemoryManagerBuilder`. When @Myasuka asked yesterday offline, I suggested to check whether any budget is set in `MemoryManagerBuilder#build`, and only add the default on-heap budget if no other budget is set. In this way, we can keep the batch test cases to use on-heap managed memory for less testing overhead, while have RocksDB state backend test cases to explicitly use off-heap managed memory only.
   
   @tillrohrmann @Myasuka 
   I'm not sure whether its necessary to set managed memory to 0 for non-RocksDB cases. Since managed memory is always lazily allocated/reserved, I think streaming test cases with non-RocksDB state backend should not reserve managed memory at all, and batch test cases should only allocate on-heap managed memory if needed (which is the same behavior as before).

----------------------------------------------------------------
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