geserdugarov commented on code in PR #10851:
URL: https://github.com/apache/hudi/pull/10851#discussion_r1527853866
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCleanConfig.java:
##########
@@ -118,28 +120,32 @@ public class HoodieCleanConfig extends HoodieConfig {
+ "the minimum number of file slices to retain in each file group,
during cleaning.");
public static final ConfigProperty<String> CLEAN_TRIGGER_STRATEGY =
ConfigProperty
- .key("hoodie.clean.trigger.strategy")
+ .key("hoodie.cleaner.trigger.strategy")
.defaultValue(CleaningTriggerStrategy.NUM_COMMITS.name())
+ .withAlternatives("hoodie.clean.trigger.strategy")
Review Comment:
@codope, I think, for making sure that alternative keys are used on the
path, it's enough to find all places using the following algorithm:
- search for all using of constants with changed keys,
- filter by presence of `get*` method in the strings, and also skip test
files,
- if the number of strings in results is not huge, check the path manually.
I've used rigrep for searching:
`rg
"AUTO_CLEAN|ASYNC_CLEAN|CLEAN_TRIGGER_STRATEGY|CLEAN_MAX_COMMITS|CLEANER_INCREMENTAL_MODE_ENABLE|FAILED_WRITES_CLEANER_POLICY|ALLOW_MULTIPLE_CLEANS|CLEAN_ASYNC_ENABLED|CLEAN_POLICY|CLEAN_RETAIN_COMMITS|CLEAN_RETAIN_HOURS|CLEAN_RETAIN_FILE_VERSIONS"
. | grep "get" | grep -v "\/src/test\/"`
and found 22 strings in 8 classes.
Classes `HoodieTableFactory`, `FlinkWriteClients`, `CleanFunction`,
`ClusteringCommitSink`, and `CompactionCommitSink`
use `getBoolean`, `getString`, and `getInteger`
of `FlinkOptions.*`,
which call `org.apache.flink.configuration.Configuration.get*` >
`getOptional` > `getRawValueFromOption` > `applyWithOption` >
```
if (valueFromExactKey.isPresent()) { ... }
else if (option.hasFallbackKeys()) { ... }
```
Alternatives in Flink options are fallback keys, so here everything is ok.
`HoodieWriteConfig` calls
`org.apache.hudi.common.config.HoodieConfig.get*` > `getRawValue` >
`getRawValueWithAltKeys`
Here, everything is ok too.
`RunCleanProcedure` only parses and set current key-value pair for the
corresponding setting.
The problem was only in `OptionsResolver`, because it checks presence of
some Hudi kernel configurations in Flink's `Configuration` using deprecate
`getString(String key, String defaultValue)`.
So, I could only [iterate over all possible string
representations](https://github.com/apache/hudi/commit/32ea10c0dc94a8684b7674129eae8725540f8617)
of the parameter, and check for the presence of any key variation. Added fix
with a corresponding unit test, and rebased on the current master.
Now, backward compatibility is supported for all keys changed in this MR.
--
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: commits-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
