Myasuka commented on code in PR #19324:
URL: https://github.com/apache/flink/pull/19324#discussion_r847029472
##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/EmbeddedRocksDBStateBackend.java:
##########
@@ -833,6 +833,12 @@ public void setWriteBatchSize(long writeBatchSize) {
this.writeBatchSize = writeBatchSize;
}
+ public double getOverlapFractionThreshold() {
Review Comment:
I think this method can be package public level.
##########
flink-state-backends/flink-statebackend-rocksdb/src/test/java/org/apache/flink/contrib/streaming/state/RocksDBIncrementalCheckpointUtilsTest.java:
##########
@@ -99,7 +99,7 @@ public void testChooseTheBestStateHandleForInitial() {
Assert.assertNull(
RocksDBIncrementalCheckpointUtils.chooseTheBestStateHandleForInitial(
keyedStateHandles,
- new KeyGroupRange(3, 5),
+ new KeyGroupRange(13, 15),
Review Comment:
The previous test is want to test that with `0.75` threshold, we might
select the init handle, however, this is not true if changed the default value
to `0.0`.
##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBConfigurableOptions.java:
##########
@@ -258,7 +258,7 @@
public static final ConfigOption<Double>
RESTORE_OVERLAP_FRACTION_THRESHOLD =
key("state.backend.rocksdb.restore-overlap-fraction-threshold")
.doubleType()
- .defaultValue(0.75)
+ .defaultValue(0.0)
.withDescription(
"The threshold of the overlap fraction between the
handle's key-group range and target key-group range. "
+ "When restore base DB, only the handle
which overlap fraction greater than or equal to *threshold* "
Review Comment:
I think we could add description of why we change the default value to `0.0`.
##########
flink-state-backends/flink-statebackend-rocksdb/src/test/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackendConfigTest.java:
##########
@@ -503,7 +503,7 @@ public void testConfigurableOptionsFromConfig() throws
Exception {
verifyIllegalArgument(RocksDBConfigurableOptions.USE_BLOOM_FILTER,
"NO");
verifyIllegalArgument(RocksDBConfigurableOptions.BLOOM_FILTER_BLOCK_BASED_MODE,
"YES");
verifyIllegalArgument(
-
RocksDBConfigurableOptions.RESTORE_OVERLAP_FRACTION_THRESHOLD, "2");
+
RocksDBConfigurableOptions.RESTORE_OVERLAP_FRACTION_THRESHOLD, "-1");
Review Comment:
I don't think we need to change this test case.
##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBIncrementalCheckpointUtils.java:
##########
@@ -53,7 +53,7 @@ private static Score stateHandleEvaluator(
(double) intersectGroup.getNumberOfKeyGroups()
/ handleKeyGroupRange.getNumberOfKeyGroups();
- if (overlapFraction < overlapFractionThreshold) {
+ if (overlapFraction <= 0 || overlapFraction <
overlapFractionThreshold) {
Review Comment:
I don't think we need to add condition `overlapFraction <= 0`.
##########
flink-state-backends/flink-statebackend-rocksdb/src/test/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackendConfigTest.java:
##########
@@ -771,6 +771,19 @@ public void testConfigureIllegalMemoryControlParameters() {
}
}
+ @Test
+ public void testConfigureRestoreOverlapThreshold() {
Review Comment:
I think we can split this test into two, one is for testing the default
value, another is for testing the customized value.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
