dmvk commented on a change in pull request #18869:
URL: https://github.com/apache/flink/pull/18869#discussion_r814543427
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/zookeeper/ZooKeeperStateHandleStore.java
##########
@@ -321,24 +316,19 @@ private boolean
indicatesPossiblyInconsistentState(Exception e) {
public IntegerResourceVersion exists(String pathInZooKeeper) throws
Exception {
checkNotNull(pathInZooKeeper, "Path in ZooKeeper");
- return getStatIfNotMarkedAsDeleted(pathInZooKeeper)
+ return getStat(pathInZooKeeper)
+ .filter(ZooKeeperStateHandleStore::isNotMarkedForDeletion)
.map(stat -> IntegerResourceVersion.valueOf(stat.getVersion()))
.orElse(IntegerResourceVersion.notExisting());
}
- private Optional<Stat> getStatIfNotMarkedAsDeleted(String path) throws
Exception {
- return getStatIfExistingInternal(path, NOT_MARKED_FOR_DELETION);
- }
-
- private Optional<Stat> nodeExistsButMightBeMarkedAsDeleted(String path)
throws Exception {
- return getStatIfExistingInternal(path, Predicates.alwaysTrue());
+ private static boolean isNotMarkedForDeletion(Stat stat) {
+ return stat != null && stat.getNumChildren() > 0;
Review comment:
nit: should never be null
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/persistence/TestingLongStateHandleHelper.java
##########
@@ -79,8 +82,17 @@ public static int getGlobalDiscardCount() {
private int numberOfDiscardCalls = 0;
+ private final AtomicReference<RuntimeException>
actualOnFirstDiscardException;
+
public LongStateHandle(long value) {
+ this(value, null);
+ }
+
+ public LongStateHandle(
+ long value, @Nullable RuntimeException
actualOnFirstDiscardException) {
Review comment:
should we rather have a callback here, this seems too specific to a
particular test. Something along the lines of
```java
@FunctionalInterface
interface PreDiscardFn {
preDiscard(int discardIdx) throws Exception();
}
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/zookeeper/ZooKeeperStateHandleStoreTest.java
##########
@@ -954,4 +1212,38 @@ public void testRemoveAllHandlesShouldRemoveAllPaths()
throws Exception {
assertThat(zkStore.getAllHandles(), is(empty()));
}
+
+ @Test
+ public void testGetAllHandlesWithMarkedForDeletionEntries() throws
Exception {
Review comment:
Sounds good, this seems to be only used by the
`SessionDispatcherLeaderProcess` and the `StateHandleStore` itself
--
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]
