rkhachatryan commented on code in PR #19679:
URL: https://github.com/apache/flink/pull/19679#discussion_r936494486
##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java:
##########
@@ -825,6 +841,38 @@ public <N, SV, SEV, S extends State, IS extends S> IS
createInternalState(
@Nonnull StateDescriptor<S, SV> stateDesc,
@Nonnull StateSnapshotTransformFactory<SEV>
snapshotTransformFactory)
throws Exception {
+ Tuple2<ColumnFamilyHandle, RegisteredKeyValueStateBackendMetaInfo<N,
SV>> registerResult =
+ tryRegisterKvStateInformation(
+ stateDesc, namespaceSerializer,
snapshotTransformFactory, false);
+ ttlCompactFiltersManager.configCompactFilter(
+ stateDesc, registerResult.f1.getStateSerializer());
+
+ return createState(stateDesc, registerResult);
Review Comment:
I think this might cause issues in the future, if a developer expects the
same behavior from the two methods; but the 2nd won't actually apply TTL
settings.
To solve this, the 2nd version could call
`ttlCompactFiltersManager.configCompactFilter` if `allowFutureMetadataUpdates
== true`.
That might seem hacky, but it conveys the current logic better IMO, which is:
only take TTL config into account if no future metadata updates (and hence,
TTL) will happen.
WDYT?
##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java:
##########
@@ -825,6 +841,38 @@ public <N, SV, SEV, S extends State, IS extends S> IS
createInternalState(
@Nonnull StateDescriptor<S, SV> stateDesc,
@Nonnull StateSnapshotTransformFactory<SEV>
snapshotTransformFactory)
throws Exception {
+ Tuple2<ColumnFamilyHandle, RegisteredKeyValueStateBackendMetaInfo<N,
SV>> registerResult =
+ tryRegisterKvStateInformation(
+ stateDesc, namespaceSerializer,
snapshotTransformFactory, false);
+ ttlCompactFiltersManager.configCompactFilter(
+ stateDesc, registerResult.f1.getStateSerializer());
+
+ return createState(stateDesc, registerResult);
Review Comment:
I think this might cause issues in the future, if a developer expects the
same behavior from the two methods; but the 2nd won't actually apply TTL
settings.
To solve this, the 2nd version could call
`ttlCompactFiltersManager.configCompactFilter` if `allowFutureMetadataUpdates
== true`.
That might seem hacky, but it conveys the current logic better IMO, which is:
only take TTL config into account if no future metadata updates (and hence,
TTL) will happen.
WDYT?
--
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]
