You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/04/11 07:43:08 UTC
[GitHub] [flink] Myasuka commented on a diff in pull request #19324: [FLINK-26757][state] Change the default value of state.backend.rocksdb.restore-overlap-fraction-threshold to 0
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org