Details

    • Type: Improvement
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.3.0
    • Fix Version/s: 1.3.0
    • Component/s: DataStream API
    • Labels:
      None

      Description

      I'm proposing to remove size() because it is a prohibitively expensive operation and users might not be aware of it. Instead of size() users can use an iterator over all mappings to determine the size, when doing this they will be aware of the fact that it is a costly operation.

      Right now, size() is only costly on the RocksDB state backend but I think with future developments on the in-memory state backend it might also become an expensive operation there.

        Issue Links

          Activity

          Hide
          aljoscha Aljoscha Krettek added a comment -

          Stefan Richter and Xiaogang Shi, what do you think about this?

          Show
          aljoscha Aljoscha Krettek added a comment - Stefan Richter and Xiaogang Shi , what do you think about this?
          Hide
          xiaogang.shi Xiaogang Shi added a comment -

          I think it's okay to remove this method. But a better choice is to use an in-memory cache to record the size of the state. That way, we can achieve constant-time implementation of the `size` method with little cost. I think this mechanism also works for the heap states in the future.

          Show
          xiaogang.shi Xiaogang Shi added a comment - I think it's okay to remove this method. But a better choice is to use an in-memory cache to record the size of the state. That way, we can achieve constant-time implementation of the `size` method with little cost. I think this mechanism also works for the heap states in the future.
          Hide
          StephanEwen Stephan Ewen added a comment -

          Doesn't the cache compromise the "out-of-core" capabilities of the RocksDB-based MapState?

          Show
          StephanEwen Stephan Ewen added a comment - Doesn't the cache compromise the "out-of-core" capabilities of the RocksDB-based MapState?
          Hide
          xiaogang.shi Xiaogang Shi added a comment -

          Stephan Ewen We just use the cache to avoid the costly scanning. It's initialized the first time the `size()` method is called. After then, the cache will be updated every time a new entry is inserted or an entry is removed. When the backend is closed, we can simply drop the cache.

          A better choice, i think, is to use a RocksDB entry to record the value of the `size`. We don't need to write the value into the entry everytime it's updated. We can update it only when taking snapshots. But this requires states to be aware of checkpointing which is missing in our current implementation.

          Show
          xiaogang.shi Xiaogang Shi added a comment - Stephan Ewen We just use the cache to avoid the costly scanning. It's initialized the first time the `size()` method is called. After then, the cache will be updated every time a new entry is inserted or an entry is removed. When the backend is closed, we can simply drop the cache. A better choice, i think, is to use a RocksDB entry to record the value of the `size`. We don't need to write the value into the entry everytime it's updated. We can update it only when taking snapshots. But this requires states to be aware of checkpointing which is missing in our current implementation.
          Hide
          aljoscha Aljoscha Krettek added a comment -

          I think keeping the cache will be quite hard: you never know whether adding an entry actually increases the size because an entry could be for a pre-existing key.

          Show
          aljoscha Aljoscha Krettek added a comment - I think keeping the cache will be quite hard: you never know whether adding an entry actually increases the size because an entry could be for a pre-existing key.
          Hide
          srichter Stefan Richter added a comment -

          I agree with Aljoscha Krettek. Unlike Java's HashMap, RocksDB does not give any feedback if a key already existed and this makes a lot of sense if you consider the LSM implementation of RocksDB. RocksDB is offering an estimate on the key-count (probably implemented as hyper-log-log), but I think they would expose the exact count if this could be easily done. The only workaround that I see to maintain a cached count is to perform a lookup before every insert, which should be prohibitively expensive.

          Show
          srichter Stefan Richter added a comment - I agree with Aljoscha Krettek . Unlike Java's HashMap, RocksDB does not give any feedback if a key already existed and this makes a lot of sense if you consider the LSM implementation of RocksDB. RocksDB is offering an estimate on the key-count (probably implemented as hyper-log-log), but I think they would expose the exact count if this could be easily done. The only workaround that I see to maintain a cached count is to perform a lookup before every insert, which should be prohibitively expensive.
          Hide
          StephanEwen Stephan Ewen added a comment -

          I think it is a good approach to offer only functions that we can implement efficiently.
          It is also much easier to add functionality later than having added functions that were not efficient/dangerous and then being stuck maintaining them.

          Show
          StephanEwen Stephan Ewen added a comment - I think it is a good approach to offer only functions that we can implement efficiently. It is also much easier to add functionality later than having added functions that were not efficient/dangerous and then being stuck maintaining them.
          Hide
          xiaogang.shi Xiaogang Shi added a comment -

          +1 to remove the size() method due to the cost implementation.

          Show
          xiaogang.shi Xiaogang Shi added a comment - +1 to remove the size() method due to the cost implementation.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user shixiaogang opened a pull request:

          https://github.com/apache/flink/pull/3462

          FLINK-5917[state] Remove size() method from MapState

          The `size()` method is removed from `MapState` because its implementation is costly in the backends.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/alibaba/flink flink-5917

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3462.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3462


          commit 6906b15ff593f46e106348aa1f5772e6b78efe74
          Author: xiaogang.sxg <xiaogang.sxg@alibaba-inc.com>
          Date: 2017-03-03T02:27:11Z

          Remove size() method from MapState


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user shixiaogang opened a pull request: https://github.com/apache/flink/pull/3462 FLINK-5917 [state] Remove size() method from MapState The `size()` method is removed from `MapState` because its implementation is costly in the backends. You can merge this pull request into a Git repository by running: $ git pull https://github.com/alibaba/flink flink-5917 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3462.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3462 commit 6906b15ff593f46e106348aa1f5772e6b78efe74 Author: xiaogang.sxg <xiaogang.sxg@alibaba-inc.com> Date: 2017-03-03T02:27:11Z Remove size() method from MapState
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StefanRRichter commented on the issue:

          https://github.com/apache/flink/pull/3462

          LGTM +1.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/3462 LGTM +1.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aljoscha commented on the issue:

          https://github.com/apache/flink/pull/3462

          LGTM! Could you please go ahead and merge, @StefanRRichter?

          Show
          githubbot ASF GitHub Bot added a comment - Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/3462 LGTM! Could you please go ahead and merge, @StefanRRichter?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StefanRRichter commented on the issue:

          https://github.com/apache/flink/pull/3462

          Yes, will do.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/3462 Yes, will do.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3462

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3462
          Hide
          srichter Stefan Richter added a comment -

          Fixed in f37507d (master)

          Show
          srichter Stefan Richter added a comment - Fixed in f37507d (master)

            People

            • Assignee:
              xiaogang.shi Xiaogang Shi
              Reporter:
              aljoscha Aljoscha Krettek
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development