Github user StefanRRichter commented on a diff in the pull request:

    https://github.com/apache/flink/pull/4798#discussion_r146166380
  
    --- Diff: 
flink-contrib/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBKeyedStateBackend.java
 ---
    @@ -235,26 +235,16 @@ public RocksDBKeyedStateBackend(
                this.instanceBasePath = 
Preconditions.checkNotNull(instanceBasePath);
                this.instanceRocksDBPath = new File(instanceBasePath, "db");
     
    -           // Clear this directory when the backend is created
    +           // Clear the base directory when the backend is created
                // in case something crashed and the backend never reached 
dispose()
    -           cleanInstanceBasePath();
    -
    -           if (!instanceBasePath.exists()) {
    +           if (instanceBasePath.exists()) {
    +                   cleanInstanceBasePath();
    --- End diff --
    
    Unfortunately, this looks incorrect now, because the directory is actually 
deleted and the `else` branch is not hit. There is also a method to just clear 
directory content, but then different methods should be used here and in 
dispose (which can actually delete the directory).
    
    Nit: could merge `else`+ `if` into `else if`.


---

Reply via email to