Skip to content

Conversation

@ltagliamonte-dd
Copy link
Contributor

@ltagliamonte-dd ltagliamonte-dd commented Mar 13, 2025

Hello Folks,
To enable SSTable sideloading for Redis types that use versioning, retrieving the metadata.version is necessary when updating an existing key.

To support this, we've introduced a new command: KMETADATA, which returns the metadata for a given key.

Looking forward to your feedback!

@git-hulk
Copy link
Member

@apache/kvrocks-committers What do you feel about this command proposal?

@torwig
Copy link
Contributor

torwig commented Mar 14, 2025

@ltagliamonte-dd Could you please explain how the data returned by this command will be used for "SSTable sideloading"? Just to understand the flow.

@ltagliamonte-dd
Copy link
Contributor Author

ltagliamonte-dd commented Mar 14, 2025

@ltagliamonte-dd Could you please explain how the data returned by this command will be used for "SSTable sideloading"? Just to understand the flow.

Sure thing @torwig, here more details:
For my understanding data types that use versioning (like sets,zset,hash) share the key version with their sub keys.

For example for hash data types (HSET Key subKey1 subKey1val1) there is a rocksdb row for the Key and an row for subKey1.
The Key and the subKey1 share the metadata.version.

When writing to kvrocks there are 2 use cases:

  • add a new key
    a new version is created for the Key and that version is inherited by subKey1
  • update an existing key
    the version needs to be retrieved from the Key and shared with the subKey1

This new CMD covers the update of an existing key.

I'm still quite new to the decode/encode part of the project please let me know if my understanding is not correct.

@torwig
Copy link
Contributor

torwig commented Mar 14, 2025

@git-hulk Could you please provide a link to the encoding schema? I can't find it :)
@ltagliamonte-dd Actually, for complex data types there is just one, let's say Final-Key that consists of Key (type + version + slot ID, etc.) followed by Subkey. For example, all the elements of HSET myset have the common prefix (the prefix has version), subkeys don't.

@ltagliamonte-dd
Copy link
Contributor Author

@torwig thanks, since my use case is just HSET i've been focusing on this function https://github.com/apache/kvrocks/blob/unstable/src/types/redis_hash.cc#L241-L296

going over the code it looks like that when the key exists, metadata.version contains the current version of the Key hash.
This version is used to construct the subkeys for the field-value pairs being set or updated.
Am i interpreting this wrong?

Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

Maybe we can have a better name, GETXXX seems not so "Redis".

To avoid collision with future Redis commands, we can prefix with something like K or KVR, e.g. KMETADATA.

@ltagliamonte-dd
Copy link
Contributor Author

To avoid collision with future Redis commands, we can prefix with something like K or KVR, e.g. KMETADATA.

love KMETADATA!

@ltagliamonte-dd ltagliamonte-dd changed the title feat(GETMETA): add new cmd GETMETA feat(KMETADATA): add new cmd KMETADATA Mar 18, 2025
@ltagliamonte-dd
Copy link
Contributor Author

@torwig @git-hulk @PragmaTwice can i please get a CR on this?

git-hulk
git-hulk previously approved these changes Mar 20, 2025
@git-hulk
Copy link
Member

@ltagliamonte-dd
Copy link
Contributor Author

@ltagliamonte
Copy link

@git-hulk @torwig @PragmaTwice can you please run the CI again it failed for a flaky test.
My PR should be good to go if you folks don't have any other comments.

git-hulk
git-hulk previously approved these changes Mar 21, 2025
@sonarqubecloud
Copy link

@PragmaTwice PragmaTwice changed the title feat(KMETADATA): add new cmd KMETADATA feat(KMETADATA): add new command KMETADATA Mar 22, 2025
@PragmaTwice PragmaTwice changed the title feat(KMETADATA): add new command KMETADATA feat(cmd): add new command KMETADATA Mar 22, 2025
@PragmaTwice PragmaTwice merged commit 74084cc into apache:unstable Mar 22, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants