Skip to content

Conversation

sihuazhou
Copy link
Contributor

What is the purpose of the change

This PR fixes the bug that the KeyedStateBackend.getKeys() does not work on RocksDB MapState.

Brief change log

  • Change RocksDBKeyedStateBackend#RocksIteratorForKeysWrapper() to let it support get keys for RocksDB MapState.

Verifying this change

  • Added StateBackendTestBase#testMapStateGetKeys() to guard the changes

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • No

@sihuazhou
Copy link
Contributor Author

CC @aljoscha

Copy link
Contributor

@aljoscha aljoscha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good! I had one code comment but you can ignore that one.

Possibly @StefanRRichter also wants to have a look but I think you can merge. We should fix this both for 1.5.x and 1.6.0.

this.keyGroupPrefixBytes = Preconditions.checkNotNull(keyGroupPrefixBytes);
this.namespaceBytes = Preconditions.checkNotNull(namespaceBytes);
this.nextKey = null;
this.preKey = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call this previousKey, just to be more clear.

@sihuazhou
Copy link
Contributor Author

@aljoscha Thanks for your quick review, will address your comments while merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants