-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add ability to copy shard via rpc calls. Remove deprecated copier service. #6502
Conversation
corylanou
commented
Apr 29, 2016
•
edited
Loading
edited
- Rebased/mergable
- Tests pass
- CHANGELOG.md updated
- Removes deprecated copier service
- adds plumbing for backup/restore via rpc
By analyzing the blame information on this pull request, we identified @e-dard, @mark-rushakoff and @gunnaraasen to be potential reviewers |
All of the plumbing was written by @benbjohnson I just made it work with the current version of this code. I'll be doing some testing to make sure it works across nodes and then I'll ask for reviews once I know it really works. |
I've decided that we should review and ship this otherwise I'll have a gigantic PR to merge before I'm done. |
// Encode success response. | ||
if err := EncodeTLV(conn, copyShardResponseMessage, &CopyShardResponse{}); err != nil { | ||
s.Logger.Printf("error writing CopyShard response: %s", err) | ||
return |
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.
return
is redundant.
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 defensively added that in case code was added in the future after the if
block. That's just my personal preference though.
I have a question I'll ask out-of-band, but aside from that LGTM 👍 |
eaa27fe
to
c3f3310
Compare
Needs a rebase but SLGTM 😉 |