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

[Draft] feat: adding copy #4038

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sanathkandikanti
Copy link

Creating a scratch pull request to get some early directional feedback.

For testing i tried moving a string and list form one database to another and it worked. Haven't written any unit tests yet as source code is still work in progress.

Once you confirm this is in the right direction i will add more functionality

Not sure how to connect a pull request to a Issue so i'm just linking it in the description: #3878

Signed-off-by: Sanath <[email protected]>
Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR. Unfortunately this command requires a non-trivial 2 hop transaction design and this PR requires substantial improvements. I also see that you mostly copied the code from the "MOVE" command without fixing the code.

@@ -786,6 +786,49 @@ OpStatus OpMove(const OpArgs& op_args, string_view key, DbIndex target_db) {
return OpStatus::OK;
}

// OpMove touches multiple databases (op_args.db_idx, target_db), so it assumes it runs
Copy link
Collaborator

Choose a reason for hiding this comment

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

OpMove -> OpCopy

Copy link
Author

Choose a reason for hiding this comment

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

Didn't spend time on docs as this was just a draft

// OpMove touches multiple databases (op_args.db_idx, target_db), so it assumes it runs
// as a global transaction.
// TODO: Allow running OpMove without a global transaction.
OpStatus OpCopy(const OpArgs& op_args, string_view key, DbIndex target_db) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy is actually a non-trivial operation that requires very good understanding of Dragonfly architecture.

This code won't work when keys reside on different thread shards. If you wish to learn more about dragonfly architecture and its transactional framework, please read the docs under docs/ folder.

Copy link
Author

Choose a reason for hiding this comment

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

I'll go through the docs and share an one-pager for this feature. Do you recommend any other resources/docs i can look at in general to understand this better?

Do you have any timeline in mind for this feature? I'm new to this domain, so will take a few weeks to deliver this.

Copy link
Author

Choose a reason for hiding this comment

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

do you have any suggested order for reading files in the docs folder?

Copy link
Collaborator

Choose a reason for hiding this comment

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

df-share-nothing.md and transaction.md are good candidates

@sanathkandikanti
Copy link
Author

Thanks for submitting this PR. Unfortunately this command requires a non-trivial 2 hop transaction design and this PR requires substantial improvements. I also see that you mostly copied the code from the "MOVE" command without fixing the code.

@romange Can you explain an example where this would break so i can reproduct it. This implementation works when copying the key from one database to another database. With some modifications it should also work for moving data from one key to another. Do you have any references for 2 hop transaction? I will spend some time to understand it better before implementing further.

@romange
Copy link
Collaborator

romange commented Nov 2, 2024

Thanks for submitting this PR. Unfortunately this command requires a non-trivial 2 hop transaction design and this PR requires substantial improvements. I also see that you mostly copied the code from the "MOVE" command without fixing the code.

@romange Can you explain an example where this would break so i can reproduct it. This implementation works when copying the key from one database to another database. With some modifications it should also work for moving data from one key to another. Do you have any references for 2 hop transaction? I will spend some time to understand it better before implementing further.

it's not very hard to find out where it breaks. part of our responsibility is to write tests and challenge our code. once you add unit tests to generic_family_test, I am sure you will see where it breaks.

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.

2 participants