weiqingy commented on PR #721: URL: https://github.com/apache/flink-agents/pull/721#issuecomment-4598232800
> Hi @weiqingy , > > It seems to me there are some misunderstanding of the TTL clean-up strategies. I checked the implementation of [StateTtlConfig](https://github.com/apache/flink/blob/master/flink-core/src/main/java/org/apache/flink/api/common/state/StateTtlConfig.java) and find that: > > 1. The cleanup strategies are additive rather than exclusive. This means a config option that only allows one value may not be expressive enough. > 2. `INCREMENTAL` and `ROCKSDB_COMPACTION_FILTER` is enabled by default. The only strategy what won't be enabled by default is `FULL_SNAPSHOT`. > > Based on these observations, I'm leaning towards not to introduce this new option. Because the whole cleanup strategy mechanism is too complex for the users to understand and tune, and the default behaviors should be good enough for most of the use cases. Unless we see a concrete need for this fine-tuning, these knobs might be unnecessary. > > WDYT? Thanks for digging into `StateTtlConfig` — you're right on both counts. I went through the Flink 2.2 bytecode to confirm: 1. **Additive, not exclusive.** `CleanupStrategies` keeps an `EnumMap<Strategies, CleanupStrategy>` alongside the `isCleanupInBackground` flag, so the strategies accumulate. A single-choice enum is the wrong model — selecting `INCREMENTAL` wouldn't turn off the RocksDB compaction filter, and vice versa. 2. **Background cleanup is on by default.** `Builder`'s constructor sets `isCleanupInBackground = true`. With that flag set, `getIncrementalCleanupStrategy()` and `inRocksdbCompactFilter()` are already active with default parameters — no explicit call required. `FULL_SNAPSHOT` is the only strategy that needs opting in. The key consequence: the scenario that motivated the issue — long-running RocksDB pipelines wanting `cleanupInRocksdbCompactFilter` — **is already covered by Flink's default**. The option wouldn't unlock anything new there, and the only knob that genuinely deviates from the default is disabling background cleanup, which is niche. So I agree there's no concrete need that justifies the added public surface and tuning complexity. I'll close this PR and the issue. Thanks for catching the misunderstanding early. -- 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]
