dawidwys commented on a change in pull request #13405:
URL: https://github.com/apache/flink/pull/13405#discussion_r491971571
##########
File path:
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/operators/InternalTimeServiceManager.java
##########
@@ -74,19 +74,16 @@
private final Map<String, InternalTimerServiceImpl<K, ?>> timerServices;
- private final boolean useLegacySynchronousSnapshots;
-
InternalTimeServiceManager(
- KeyGroupRange localKeyGroupRange,
- KeyContext keyContext,
- PriorityQueueSetFactory priorityQueueSetFactory,
- ProcessingTimeService processingTimeService, boolean
useLegacySynchronousSnapshots) {
Review comment:
It can. Let me explain it though, why I think it is unnecessary in the
ctor. (There is one thing I forgot in the PR, I wanted to make the
`snapshotStateForKeyGroup` private. It is used only in the
`InternalTimeServiceManager#snapshotState`.)
The flag is used only in the `snapshotStateForKeyGroup` which is used only
in the `snapshotState` method. The `snapshotState` method looks like:
```
public void snapshotState(
KeyedStateBackend<?> keyedStateBackend,
StateSnapshotContext context,
String operatorName) throws Exception {
//TODO all of this can be removed once heap-based timers are
integrated with RocksDB incremental snapshots
if (keyedStateBackend instanceof AbstractKeyedStateBackend &&
((AbstractKeyedStateBackend<?>)
keyedStateBackend).requiresLegacySynchronousTimerSnapshots()) {
....
snapshotStateForKeyGroup(new
DataOutputViewStreamWrapper(out), keyGroupIdx);
....
}
}
```
Effectively this flag is checked twice. Once in the preconditions against
the value passed in the ctor and the second time in the `snapshotState` method
against the value set in the passed `KeyedStateBackend`. In my opinion the one
time in the `snapshotState` is enough.
The only case when the condition in `snapshotStateForKeyGroup` could fail is
if we created the `InternalTimeServiceManager` from a different
`KeyedStateBackend` than we use for snapshotting. I am quite sure this would
let to way more problems than only this precondition failling.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
