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

Add ability to copy shard via rpc calls. Remove deprecated copier service. #6502

Merged
merged 4 commits into from
May 10, 2016

Conversation

corylanou
Copy link
Contributor

@corylanou corylanou commented Apr 29, 2016

  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Removes deprecated copier service
  • adds plumbing for backup/restore via rpc

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @e-dard, @mark-rushakoff and @gunnaraasen to be potential reviewers

@corylanou
Copy link
Contributor Author

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.

@corylanou corylanou changed the title WIP - Copy shard Copy shard Apr 29, 2016
@corylanou
Copy link
Contributor Author

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
Copy link
Contributor

Choose a reason for hiding this comment

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

return is redundant.

Copy link
Contributor

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.

@e-dard
Copy link
Contributor

e-dard commented Apr 29, 2016

I have a question I'll ask out-of-band, but aside from that LGTM 👍

@corylanou corylanou changed the title Copy shard Add ability to copy shard via rpc calls. Remove deprecated copier service. May 2, 2016
@corylanou corylanou force-pushed the copy-shard branch 2 times, most recently from eaa27fe to c3f3310 Compare May 9, 2016 16:10
@e-dard
Copy link
Contributor

e-dard commented May 10, 2016

Needs a rebase but SLGTM 😉

@corylanou corylanou merged commit 639b0d7 into master May 10, 2016
@corylanou corylanou deleted the copy-shard branch May 10, 2016 13:55
@timhallinflux timhallinflux added this to the 0.13.0 milestone Dec 20, 2016
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.

None yet

5 participants