Skip to content

Feature/observe backup and restore #1115

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

Merged
merged 13 commits into from
Jan 8, 2015

Conversation

robertlyson
Copy link
Contributor

Let's summarize what has been done so far:

  • observable for snapshot and restore features
  • ElasticClient.SnapshotObservable uses under the hood ElasticClient.SnapshotStatus
  • ElasticClient.RestoreObservable uses ElasticClient.RecoveryStatus
  • status is reported back with defined time period (thanks @Mpdreamz for help with Timer 👍 )

One integration test has been written for this feature(SnapshotRestoreObservable), but it is failing so far. Seems to be that the problem is in the ElasticClient.RecoveryStatus, which returns recovery status as done all the time.

Feel free to take over this issue - hope I'll find some time before next release to be able to close this topic together with you.

@gmarz
Copy link
Contributor

gmarz commented Dec 9, 2014

This is awesome! Thanks @robertlyson

@gmarz gmarz added the WIP label Dec 9, 2014
@robertlyson
Copy link
Contributor Author

Now I see where the problem is.

In the integration test, I'm making restore request with index name replacement option, but I'm checking recovery status for original index name.

I'll push this fix soon.

@Mpdreamz
Copy link
Member

Hi @robertlyson I see you amended the PR 👍

Please ping us when you are ready for us to review, can't wait to 🚢 🇮🇹

…hotobservable have been moved to responses directory
@robertlyson
Copy link
Contributor Author

Hello @Mpdreamz,

I think, more reliable tests is the last think to do for this feature. I'm still thinking about this, but good solution didn't pop up yet.
Current test still depends on machine which runs tests. Maybe you will find other way to deal with this.

From minor issues:

  1. I put Observer<T>, SnapshotObservable and RestoreObservable in the Nest\Domain\Responses directory. Couldn't find better place.

So I guess we can start review, let me know in case you will find some issues :)

@robertlyson
Copy link
Contributor Author

Hello,

Finally, I've added several unit tests for this feature - it has been done by means of introduction of Humble Object.

I think the PR can be reviewed right now, so feel free to put your comments :)

@Mpdreamz Mpdreamz self-assigned this Jan 8, 2015
@Mpdreamz Mpdreamz merged commit 713b41b into elastic:develop Jan 8, 2015
@Mpdreamz
Copy link
Member

Mpdreamz commented Jan 8, 2015

Small nags,

  • spaces instead of tabs (please use editorconfig)
  • Observer too generic, introduced SnapshotObserver() and RestoreObserver() in the same vain as the already existing ReindexObserver()

other then that amazing work @robertlyson ! 👍 Love the unit tests and thank you for creating a proper integration test as well, made reviewing this so much easier. Really appreciated.

@robertlyson robertlyson deleted the feature/ObserveBackupAndRestore branch January 8, 2015 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants