-
Notifications
You must be signed in to change notification settings - Fork 76
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
Support copy_object for log based replication #7349
Conversation
7cd87fd
to
750a48f
Compare
811cfef
to
9834745
Compare
3647f9c
to
d8f11cb
Compare
ee3325f
to
7221560
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
src/api/replication_api.js
Outdated
@@ -14,11 +14,11 @@ module.exports = { | |||
|
|||
methods: { | |||
|
|||
move_objects_by_type: { | |||
copy_objects_by_type: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the copy_type is not a requirement anymore, I don’t see a reason to preserve the _by_type
suffix.
copy_objects_by_type: { | |
copy_objects: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dannyzaken Can you pls confirm on this, I am not sure of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on that. Either way will work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think for now we can keep it as same if in future we will be implementing other types of server side copy. Otherwise we can remove it @liranmauda wdyt its a very small change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should change the api itself, just it's name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liranmauda Done, PTAL.
Signed-off-by: Aayush Chouhan <aayush.chouhan97@gmail.com>
Explain the changes