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

fix #1970 - add support for RESTORE #2535

Conversation

Moshe-RS
Copy link
Contributor

@Moshe-RS Moshe-RS commented Jun 11, 2023

Description

Currently, the DUMP command exists in the client, but there is no way to update the server with that dumped values.
The current pull request adds the RESTORE functionality and allows this.
It follows issue #1970 request.

Describe your pull request here

  • A new file that builds the RESTORE command line arguments.
  • Update to the general client's commands list.
  • A test file for the restore functionality (unit + integration).

Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

@Moshe-RS Moshe-RS marked this pull request as draft June 11, 2023 14:10
@Moshe-RS Moshe-RS force-pushed the issue-#1970-Add-support-for-redis-command-RESTORE branch from 885888a to ccddaba Compare June 14, 2023 13:35
@Moshe-RS Moshe-RS marked this pull request as ready for review June 14, 2023 13:36
@Moshe-RS
Copy link
Contributor Author

@leibale, should this pair (DUMP & RESTORE) get a specific section in the README, or is the current text enough?

@leibale
Copy link
Collaborator

leibale commented Jun 14, 2023

@Moshe-RS These commands are for "advanced users/use cases", most users will never use them. I don't think we need a section in the readme, but an example in the examples folder will be awesome :)

@Moshe-RS Moshe-RS force-pushed the issue-#1970-Add-support-for-redis-command-RESTORE branch from ccddaba to dd806d4 Compare June 15, 2023 11:22
@dtikhonov
Copy link

It would be great if these functions supported streams so that they could be used with large keys.

@leibale
Copy link
Collaborator

leibale commented Jun 27, 2023

@dtikhonov Currently redis (the server) does not RESP3 streaming, which means to be able to stream you'll have to know the size of the string/buffer ahead of time, which makes it not so useful. Once Redis will implement "streamed strings", we'll be able to support that from the client side as well.

@Moshe-RS
Regarding the PR itself - overall looks good, I'll merge it soon. :)
Thanks for contributing!

@dtikhonov
Copy link

@leibale I just meant something like this: #2552

@leibale
Copy link
Collaborator

leibale commented Jun 28, 2023

@dtikhonov I thought you want to stream values into redis, not the other way around... I really like this idea, and it could fit nicely with the new typeMapping feature in v5. The only problem I see is what to do when a string is inside a nested data type (array/map)? Maybe just like with map keys and set members, when decoding BlobString to Stream, if the BlobString is in a nested data type, it'll be decoded to string (or maybe Buffer make more sense?) instead.

WDUT?

@dtikhonov
Copy link

dtikhonov commented Jun 28, 2023

I was thinking that both DUMP and RESTORE could benefit from ability to stream that. The PR was just to show one part of it. (It was simple to write because I was just looking at that part of the code recently.)

The only problem I see is what to do when a string is inside a nested data type

We only have to deal with it if we want to use streams in type maps. Is it useful to type all strings as streams in an instance of a Redis client?

The API I envision would go something like this:

redis.dumpAsStream(key).pipe(outputSteam);

redis.restore(key, 0, inputStream);

/* Anything can be a stream, the client code is smart enough to do this as well: */
redis.hset("foo", "bar", inputStream);

@leibale
Copy link
Collaborator

leibale commented Jun 29, 2023

@dtikhonov

Streaming to Redis is problematic at the moment because Redis expects an array of "blob strings" as input, each blob string is encoded as "${length}\r\n{value}\r\n", so we cannot start streaming unless we know the size of the stream in advance.. (see encoder.ts for reference) We'll be able to do it only when "streamed strings" will be implemented on the server side.

Streaming from Redis is something we can do, but I think that string is the only "streamable" data type, how would you stream a map?

Regarding "Is it useful to type all strings as streams in an instance of a Redis client?" you don't have to do it for the whole client, you'll be able to do:

const client = createClient();

const streamClient = client.withTypeMapping({
  [RESP_TYPES.BLOB_STRING]: ReadableStream
});

await client.get('key'); // Promise<string>
await streamClient.get('key'); // Promise<ReadableStream>

Hope this is clearer...

@leibale leibale changed the title Issue #1970 add support for redis command restore fix #1970 - add support for RESTORE Sep 18, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05% 🎉

Comparison is base (a217cc1) 95.71% compared to head (c5145b9) 95.77%.
Report is 2 commits behind head on master.

❗ Current head c5145b9 differs from pull request most recent head 4fbb80e. Consider uploading reports for the commit 4fbb80e to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2535      +/-   ##
==========================================
+ Coverage   95.71%   95.77%   +0.05%     
==========================================
  Files         458      459       +1     
  Lines        4531     4544      +13     
  Branches      506      510       +4     
==========================================
+ Hits         4337     4352      +15     
+ Misses        127      126       -1     
+ Partials       67       66       -1     
Files Changed Coverage Δ
packages/client/lib/cluster/commands.ts 100.00% <100.00%> (ø)
packages/client/lib/commands/RESTORE.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leibale leibale merged commit 01ca54e into redis:master Sep 18, 2023
12 checks passed
@leibale
Copy link
Collaborator

leibale commented Sep 19, 2023

@Moshe-RS [email protected]/@redis/[email protected] is on npm 🎉

@Moshe-RS Moshe-RS deleted the issue-#1970-Add-support-for-redis-command-RESTORE branch September 22, 2023 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants