Add COPY command by swamp0407 · Pull Request #7953 · redis/redis

swamp0407 · 2020-10-24T07:22:59Z

I'm not used to using git and I'm not familiar with how to do PR, so I'm sorry if I made a mistake.

Syntax:

COPY <key> <new-key> [DB <dest-db>] [REPLACE]

Co-authored-by: tmgauss

I'm not very good at English, so there may be something wrong with the grammar of the comments in the code, etc. I'm sorry, but please check.

fix indent fix some format add test units for COPY command

src/server.c

madolson

I'll take a deeper look through all the copying tomorrow but I added some high level comments.

I think I fixed the test with this PR: #7966. Redis ACLs are being simplified in weird ways.

src/db.c

madolson · 2020-10-27T04:44:50Z

tests/unit/keyspace.tcl

+        assert {[r get mynewkey] eq "foobar"}
+        r flushdb
+    }
+


It would be nice to add a test for all the different types. You can use debug digest to validate that the key/values are the same.

Looks like the stream digest doesn't include consumer group information, probably worth expanding on that.

i'm not certain we want digest to include it.
there are commands that modify these which consider them metadata.
IIRC they do perform dirty++ and are propagated to replicas and RDB/AOF, and only avoid doing signalModifyKey (invalidating WATCH), so maybe that means that they should indeed be in the digest.
@guybe7 anything to add?

since CGs go in the AOF and RDB i think they should be included in the digest

oranagra · 2020-10-27T09:49:06Z

just mentioning that i didn't go over the code yet (just took a quick look at the comments @madolson gave).

p.s. maybe in a followup PR, you can add module API callback so that modules can provide cloning functionality. but let's put that aside for now.

madolson

Most of it reviewed now! Generally looks good.

src/db.c

src/object.c

src/db.c

src/object.c

src/stream.h

src/db.c

madolson · 2020-10-28T00:47:39Z

tests/unit/keyspace.tcl

+        assert {[r get mynewkey] eq "foobar"}
+        r flushdb
+    }
+


Looks like the stream digest doesn't include consumer group information, probably worth expanding on that.

src/object.c

swamp0407 · 2020-10-28T23:18:38Z

thanks @madolson @oranagra @guybe7 !
I fixed the code, but I wasn't sure how to fix things like removing the fast flag and keeping the LRU.
Please review.

oranagra · 2020-10-29T15:02:33Z

@swamp0407 you did remove the fast flag (added use-memory instead).
and i think the conclusion from the discussion was that we want it to behave like RESTORE, so we we use dbDelete and dbAdd (don't keep the LRU).
both of these aspects are fine in the last version of the code in that respect (unless we hear other concerns / opinions that support changing that).
I see you changed to lookupKeyRead, which is wrong IMHO, let's wait for @madolson to respond in that discussion.

madolson · 2020-11-04T18:28:51Z

The lookupKeyRead behavior is weird, but it looks like it's well established in other places, so I guess we should keep that consistent. Still a couple of other open comments. @swamp0407

src/object.c

swamp0407 · 2020-11-06T12:51:38Z

thanks @oranagra @madolson
In addition to the areas you suggested I should fix, I have also changed the code that copies the inset object.

Should I also do a fix to include the consumer group information in the digest of the stream key? I'm not sure I can implement it correctly.

oranagra

haven't yet reviewed the streams code.

i think the fix for DEBUG DIGEST and steams can be a separate PR.
unless this one really needs it in order to validate correct result.
maybe you can use XINFO FULL instead for now.

src/object.c

src/db.c

swamp0407 · 2020-11-08T02:41:12Z

Thank you for reviewing my code. @oranagra

Please check it out as I have corrected it.
I have also fixed my mistake that I found in the dupStream function.

And, I don't need to use the stream consumer group's digest, so I won't touch it.

oranagra

few more comments..
btw, it may be a good idea to move all the 'dupXxxxObject` functions to their respective type c files eventually.
all this type specific logic sounds like it shouldn't be in object.c.
but let's do that after we're done with the other comments, for an easier review of everyone involved.

src/object.c

swamp0407 · 2020-11-08T13:21:31Z

thanks @oranagra
I have corrected my codes.

I wrote the code in reference to streamReplyWithRange in t_stream.c, but I didn't understand the streamReplyWithRange function correctly, so the part about adding nack to the group and consumer pel got complicated.

After reading the code again, I thought that the same key nack would never go into multiple people's pel.

I also fixed the part about creating a new consumer.
Let me know if there is any problems with this fix.

oranagra · 2020-11-08T13:42:15Z

@swamp0407 looks like i responded to your last push before you submitted your last message.
AFAIK the entries in the PEL of the group and consumers point to the same memory (see rdb loading).

itamarhaber · 2020-11-10T14:14:09Z

Before we seal this one, I have a couple of notes:

Should this be a multi-key command? I'm torn between liking the simplicity (e.g. MOVE), my general dislike of multi-key commands but acknowledging there may be a future (see MIGRATE).
Should this support a DELETE (or similar) option to delete the source key? This would make this into an uber-command that could eventually replace/deprecate MOVE and RENAME
The current implementation copies the source's TTL, if set. I agree with this default, but perhaps we'd like to add a NOTTL (or similar) option.

oranagra · 2020-11-10T14:31:50Z

it must be a multi-key command, unless we only want to let it copy a key to another db (keeping it's name), which i think is a big miss
i would have considered it a good idea to add a DELETE argument if they where sharing code, but the fact is that if we do that, it'll just be a shortcut to the MOVE/RENAME implementation which is a fast command. so i'm not sure that's wise. but anyway, we can always add more arguments in the future.
i guess we can add NOTTL, EX <ttl>, and even KEEPTTL. not sure if that's needed, but either way we can add later.

itamarhaber · 2020-11-10T14:36:22Z

WRT 1, I guess I meant variadic keys, something like COPY SOURCE key [...] DEST key [...]

oranagra · 2020-11-10T14:40:53Z

ohh...
i don't see the value of that (just sending it inside a MULTI will do the same thing, no disadvantages).
has anyone ever requested such a thing for MOVE or RENAME?

madolson · 2020-11-10T17:57:07Z

I don't like the multi-key approach when there are flags involved, it really complicates the command. As oran said you can always multi-exec them. I also agree that we can add flags later as needed.

From my understanding the follows up here are:

Reorganize the functions so that they are located in their respective types.
Extend the debug digest functionality so that it also includes consumer groups.

Since neither of these require a major consensus, I am OK with the current iteration. @redis/core-team conensus?

src/db.c

Co-authored-by: Itamar Haber <[email protected]>

oranagra

i went over all the comments to check that we didn't forget anything, found another minor issue.

other than that:

revert the additional "del" keyspace notification
and move all the "dupXxxx" functions to their respective C files. try to find the correct place in each C file for this kind of API (not part of the commands section), and make sure to give them the right prefix. please do this refactory in a separate commit, so it'll be easy to review actual changes.

after that, i guess we're ready to merge this.

oranagra · 2020-11-11T19:43:21Z

src/db.c

+        } else if (!strcasecmp(c->argv[j]->ptr, "db") && additional >= 1) {
+            if (getLongLongFromObject(c->argv[j+1], &dbid) == C_ERR ||
+                dbid < INT_MIN || dbid > INT_MAX ||
+                selectDb(c, dbid) == C_ERR)


this trick of using selectDb to check if the dbid is in range is not very good.
if we exit with syntax error on the next argument, the selected db remains the dest db.
either re-select the original db right away, or just match the dbid range manually.

This reverts commit 893046f.

oranagra · 2020-11-17T10:03:25Z

@swamp0407 thank you. merged.

Syntax: COPY <key> <new-key> [DB <dest-db>] [REPLACE] No support for module keys yet. Co-authored-by: tmgauss Co-authored-by: Itamar Haber <[email protected]> Co-authored-by: Oran Agra <[email protected]>

add Copycommand

972d035

fix indent fix some format add test units for COPY command

madolson self-requested a review October 26, 2020 19:53

madolson reviewed Oct 26, 2020

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

fix flag of command

821e9cd

madolson reviewed Oct 27, 2020

View reviewed changes

src/db.c Outdated Show resolved Hide resolved

madolson reviewed Oct 27, 2020

View reviewed changes

src/db.c Outdated Show resolved Hide resolved

madolson reviewed Oct 27, 2020

View reviewed changes

madolson suggested changes Oct 28, 2020

View reviewed changes

oranagra reviewed Nov 4, 2020

View reviewed changes

src/object.c Outdated Show resolved Hide resolved

swamp0407 force-pushed the add_copy_command branch from 3ff83a5 to 6578c90 Compare November 6, 2020 12:21

modify functions in object.c and add more tests.

35e990d

swamp0407 force-pushed the add_copy_command branch from 6578c90 to 35e990d Compare November 6, 2020 13:00

add tests

3abca57

oranagra reviewed Nov 7, 2020

View reviewed changes

src/object.c Outdated Show resolved Hide resolved

src/object.c Outdated Show resolved Hide resolved

src/object.c Outdated Show resolved Hide resolved

src/db.c Outdated Show resolved Hide resolved

fix some mistakes

22bcc37

swamp0407 force-pushed the add_copy_command branch from 4002ad2 to 22bcc37 Compare November 8, 2020 02:57

oranagra reviewed Nov 8, 2020

View reviewed changes

src/object.c Outdated Show resolved Hide resolved

src/object.c Outdated Show resolved Hide resolved

src/object.c Outdated Show resolved Hide resolved

oranagra reviewed Nov 8, 2020

View reviewed changes

src/object.c Outdated Show resolved Hide resolved

mp911de mentioned this pull request Nov 8, 2020

Add support for COPY command redis/lettuce#1508

Closed

fix some functions

ad02af3

swamp0407 force-pushed the add_copy_command branch from 1efd8df to ad02af3 Compare November 8, 2020 13:01

fix dupStream

26edaf2

oranagra linked an issue Nov 10, 2020 that may be closed by this pull request

COPY command #6599

Closed

madolson previously approved these changes Nov 10, 2020

View reviewed changes

itamarhaber reviewed Nov 10, 2020

View reviewed changes

src/db.c Show resolved Hide resolved

itamarhaber mentioned this pull request Nov 11, 2020

Adds the COPY command redis/redis-doc#1439

Merged

Update src/db.c

893046f

Co-authored-by: Itamar Haber <[email protected]>

swamp0407 dismissed madolson’s stale review via 893046f November 11, 2020 00:39

itamarhaber previously approved these changes Nov 11, 2020

View reviewed changes

oranagra reviewed Nov 11, 2020

View reviewed changes

swamp0407 added 7 commits November 12, 2020 08:27

Revert "Update src/db.c"

601dbe8

This reverts commit 893046f.

move dupZsetObject

431b9e9

move dupListObject

11605fb

move dupHashObject

5c82918

move dupSetObject

9144594

move dupStreamObject

523605c

fix db.c

3a562f6

swamp0407 dismissed itamarhaber’s stale review via 3a562f6 November 12, 2020 01:30

oranagra approved these changes Nov 12, 2020

View reviewed changes

itamarhaber approved these changes Nov 15, 2020

View reviewed changes

yossigo approved these changes Nov 17, 2020

View reviewed changes

oranagra merged commit ea7cf73 into redis:unstable Nov 17, 2020

oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Dec 9, 2020

jiekun mentioned this pull request Dec 21, 2020

Redis 6.2 New Commands Check List redis/redis-py#1434

Closed

18 tasks

oranagra mentioned this pull request Jan 13, 2021

Redis 6.2 RC1. #8187

Merged

Conversation

swamp0407 commented Oct 24, 2020

Uh oh!

Uh oh!

madolson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

madolson Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

madolson Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

oranagra Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

guybe7 Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

oranagra commented Oct 27, 2020

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

swamp0407 commented Oct 28, 2020

Uh oh!

oranagra commented Oct 29, 2020

Uh oh!

madolson commented Nov 4, 2020

Uh oh!

Uh oh!

swamp0407 commented Nov 6, 2020

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swamp0407 commented Nov 8, 2020

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swamp0407 commented Nov 8, 2020

Uh oh!

oranagra commented Nov 8, 2020

Uh oh!

itamarhaber commented Nov 10, 2020

Uh oh!

oranagra commented Nov 10, 2020

Uh oh!

itamarhaber commented Nov 10, 2020

Uh oh!

oranagra commented Nov 10, 2020

Uh oh!

madolson commented Nov 10, 2020

Uh oh!

Uh oh!

oranagra left a comment

madolson left a comment •

edited

Loading