xuyangzhong commented on code in PR #23980:
URL: https://github.com/apache/flink/pull/23980#discussion_r1879409013
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/utils/TableConfigUtils.java:
##########
@@ -105,16 +104,6 @@ public static ZoneId getLocalTimeZone(ReadableConfig
tableConfig) {
return ZoneId.of(zone);
}
- /**
- * Similar to {@link TableConfig#getMaxIdleStateRetentionTime()}.
- *
- * @see TableConfig#getMaxIdleStateRetentionTime()
- */
- @Deprecated
- public static long getMaxIdleStateRetentionTime(ReadableConfig
tableConfig) {
Review Comment:
I believe that this utility method should be retained for the following
reasons:
1. Based on the context of
[FLINK-16835](https://issues.apache.org/jira/browse/FLINK-16835) and
[FLINK-18555](https://issues.apache.org/jira/browse/FLINK-18555), some
operators have not fully transitioned to using `StateTtlConfig` for min/max
state retention time (e.g. over aggs and temporal joins). Therefore, the
min/max state retention time needs to be temporarily retained as an internal
variable.
2. Logic like `config.getStateRetentionTime() * 3 / 2` is scattered
throughout the code, making it difficult to maintain.
3. Since `TableConfigUtils` is a non-public API, users won't notice its
presence, so it's fine to keep it for now.
I would consider deleting this method only if we can completely eliminate
the min/max state retention time across all code and fully replace it with
`StateTtlConfig`. What do you think?
--
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]
