feat: add copy cmd #5032
feat: add copy cmd #5032
Conversation
BorysTheDev
requested review from
romange,
kostasrim,
vyavdoshenko and
adiholden
| @@ -300,12 +300,14 @@ OpStatus OpPersist(const OpArgs& op_args, string_view key); | |||
|
|
|||
| class Renamer { | |||
| public: | |||
| Renamer(Transaction* t, std::string_view src_key, std::string_view dest_key, unsigned shard_count) | |||
| Renamer(Transaction* t, std::string_view src_key, std::string_view dest_key, unsigned shard_count, | |||
There was a problem hiding this comment.
I do not see the search index updating.
Now, RENAME removes the search index for the source file. Please add a simple test and verify the search index for both the source and the destination after the COPY operation is executed.
There was a problem hiding this comment.
Could you clarify what search index are you talking about
There was a problem hiding this comment.
You can execute FT.SEARCH before copy and after.
Please verify that the source document can be found before and after the copy. Also, the destination can't be found before and can be found after.
There was a problem hiding this comment.
Please take a look at the PR: #4995
It fixes the particular issue, but it unconditionally removes the search index for the source file.
I guess you have to add the condition, is it a copy or a rename?
Also, don't forget to add the test to verify it.
There was a problem hiding this comment.
But I don't use that code
There was a problem hiding this comment.
What you changed is the single shard optimization that we have in Rename command that @BorysTheDev is not using in his code.
Normal rename (without) the optimization mentioned above always:
- Serializes the key via dump command
- Deserializes it again by loading it to the destination shard
Both of these flows handle the indexes transparently so there is no question of correctness here. We also have tests for this behaviour see SearchFamilyTest::DocsEditing and as this is the exact same flow with a different name there is no point to act any further.
There was a problem hiding this comment.
@vyavdoshenko I've checked everything works as @kostasrim said
| void GenericFamily::Copy(CmdArgList args, const CommandContext& cmd_cntx) { | ||
| CmdArgParser parser(args); | ||
| auto [k1, k2] = parser.Next<std::string_view, std::string_view>(); | ||
| bool replace = parser.Check("REPLACE"); |
There was a problem hiding this comment.
We don't support [DB destination-db] option right ? Also I guess, we parse COPY k1 k2 NONSENSE and allow it (including COPY k1 k2 nonsense more nonsense here)
There was a problem hiding this comment.
yes we don't support [DB destination-db], but I forget to add check for nonsense
|
|
||
| const char* keys[2] = {"b", "x"}; | ||
| auto ren_fb = pp_->at(0)->LaunchFiber([&] { | ||
| for (size_t i = 0; i < 200; ++i) { |
fixes: #3878
I've implemented COPY via the rename mechanism. To my mind in the future if we decide to implement the whole functionality of COPY it will be good idea to unite code of the MOVE COPY RENAME commands together