Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support of the command DUMP #2227

Merged
merged 2 commits into from
Apr 8, 2024
Merged

Add support of the command DUMP #2227

merged 2 commits into from
Apr 8, 2024

Conversation

AntiTopQuark
Copy link
Contributor

Close #1946

Command Desc:

  1. Adds support for the Dump command, which can be used as dump key.
  2. Currently, the supported data types include: String, Hash, List, Set, ZSet. Redis also supports Stream and Module types; however, due to significant differences between kvrocks and Redis, Stream and Module types are not supported at this time.
  3. Since kvrocks does not support encoding methods such as listpack and lzf, most types calculate the dump payload string using the RAW method.

Redis Documentation: https://redis.io/commands/dump/

@torwig
Copy link
Contributor

torwig commented Apr 6, 2024

@AntiTopQuark Running the ./x.py format should fix the linting issues.

@git-hulk
Copy link
Member

git-hulk commented Apr 7, 2024

Since kvrocks does not support encoding methods such as listpack and lzf, most types calculate the dump payload string using the RAW method.

I feel good about this, and we can make it work first and then improve.

src/storage/rdb.cc Outdated Show resolved Hide resolved
}

/* CRC64 */
std::string &output = static_cast<RdbStringStream *>(stream_.get())->GetInput();
Copy link
Member

Choose a reason for hiding this comment

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

If you want to assume it must be an RdbStringStream here, you can add a CHECK statement.

Copy link
Member

Choose a reason for hiding this comment

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

But TBH, to support output files rather than strings, we need a better solution here.

Copy link
Member

Choose a reason for hiding this comment

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

For the Redis dump command, it needs to echo the result to the client, so it should be good to keep in memory. Not sure if I misunderstood @PragmaTwice point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want to assume it must be an RdbStringStream here, you can add a CHECK statement.

I added a member attribute named type_ to RdbStream, and I ensure its safety by checking it before performing a static_cast

Copy link
Contributor Author

@AntiTopQuark AntiTopQuark Apr 7, 2024

Choose a reason for hiding this comment

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

But TBH, to support output files rather than strings, we need a better solution here.

Is it absolutely necessary to dump a value into a file?
If the goal is simply to dump the contents of specific keys, this can be readily achieved by redirecting the output of redis-cli to a designated file.
For dumping an entire RDB, might this feature be unnecessary? Directly utilizing kvrocks2redis would suffice. This requirement has been previously discussed: #1001 (comment)

src/storage/rdb.cc Outdated Show resolved Hide resolved
src/storage/rdb.cc Outdated Show resolved Hide resolved
Copy link

sonarqubecloud bot commented Apr 7, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link
Member

@jihuayu jihuayu left a comment

Choose a reason for hiding this comment

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

The beautiful code that ignites a delightful start to my day.
Thank you!

@git-hulk git-hulk changed the title Support DUMP Command Add support of the command DUMP Apr 8, 2024
@AntiTopQuark AntiTopQuark requested a review from PragmaTwice April 8, 2024 08:26
@git-hulk git-hulk merged commit 738b6cd into apache:unstable Apr 8, 2024
31 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.

Add support of the command DUMP
5 participants