Myasuka commented on a change in pull request #13688:
URL: https://github.com/apache/flink/pull/13688#discussion_r518198122
##########
File path:
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBResourceContainer.java
##########
@@ -103,6 +103,19 @@ public DBOptions getDbOptions() {
return opt;
}
+ /**
+ * Gets write buffer manager capacity.
+ *
+ * @return the capacity, or null if write buffer managed is not enabled
(not using managed memory).
Review comment:
This doc is not so accurate, we could not use managed memory but still
enable write buffer manager, e.g. configuring
`state.backend.rocksdb.memory.fixed-per-slot` and set
`state.backend.rocksdb.memory.managed` as false.
##########
File path:
flink-state-backends/flink-statebackend-rocksdb/src/test/java/org/apache/flink/contrib/streaming/state/RocksDBOperationsUtilsTest.java
##########
@@ -78,6 +79,25 @@ public void testPathExceptionOnWindows() throws Exception {
}
}
+ @Test
+ public void testSanityCheckArenaBlockSize() {
+ long testWriteBufferSize = 56 * 1024 * 1024L;
+ long testDefaultArenaSize =
RocksDBMemoryControllerUtils.calculateRocksDBDefaultArenaBlockSize(testWriteBufferSize);
+ long testWriteBufferCapacityBoundary = testDefaultArenaSize * 8
/ 7;
+ assertThat("The sanity check result is incorrect with default
arena block size",
+
RocksDBOperationUtils.sanityCheckArenaBlockSize(testWriteBufferSize, 0,
testWriteBufferCapacityBoundary),
+ is(true));
+ assertThat("The sanity check result is incorrect with default
arena block size given as argument",
Review comment:
The same as above.
##########
File path:
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackend.java
##########
@@ -557,8 +557,7 @@ public CheckpointStorage createCheckpointStorage(JobID
jobId) throws IOException
metricGroup,
stateHandles,
keyGroupCompressionDecorator,
- cancelStreamRegistry
- )
+ cancelStreamRegistry)
Review comment:
This change seems not so necessary in this PR.
##########
File path:
flink-state-backends/flink-statebackend-rocksdb/src/test/java/org/apache/flink/contrib/streaming/state/RocksDBOperationsUtilsTest.java
##########
@@ -78,6 +79,25 @@ public void testPathExceptionOnWindows() throws Exception {
}
}
+ @Test
+ public void testSanityCheckArenaBlockSize() {
+ long testWriteBufferSize = 56 * 1024 * 1024L;
+ long testDefaultArenaSize =
RocksDBMemoryControllerUtils.calculateRocksDBDefaultArenaBlockSize(testWriteBufferSize);
+ long testWriteBufferCapacityBoundary = testDefaultArenaSize * 8
/ 7;
+ assertThat("The sanity check result is incorrect with default
arena block size",
Review comment:
I think the message should be `should pass` instead of `is incorrect`.
##########
File path:
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBMemoryControllerUtils.java
##########
@@ -95,4 +95,51 @@ static Cache createCache(long cacheCapacity, double
highPriorityPoolRatio) {
static WriteBufferManager createWriteBufferManager(long
writeBufferManagerCapacity, Cache cache) {
return new WriteBufferManager(writeBufferManagerCapacity,
cache);
}
+
+ /**
+ * Calculate the default arena block size as RocksDB calculates it in
+ * <a
href="https://github.com/ververica/frocksdb/blob/49bc897d5d768026f1eb816d960c1f2383396ef4/db/column_family.cc#L196-L201";>
Review comment:
We should still use current `github.com/dataArtisans/frocksdb` link here.
----------------------------------------------------------------
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]
