Skip to content

Recovery merge - #679

Open
abudnik wants to merge 1 commit into
reverbrain:masterfrom
abudnik:recovery_merge
Open

Recovery merge#679
abudnik wants to merge 1 commit into
reverbrain:masterfrom
abudnik:recovery_merge

Conversation

@abudnik

@abudnik abudnik commented Dec 15, 2015

Copy link
Copy Markdown
Member

recovery: merge: removed obsolete Recovery code; used server-send, copy-iter everywhere

This PR sends greetings to PR #678

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

enumerate accepts optional parametr, which sets initial value of index. It's 0 by default.

enumerate(iterator, 1)

to remove index + 1

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed

@abudnik

abudnik commented Dec 17, 2015

Copy link
Copy Markdown
Member Author

Fixed: 20e5376

@agend

agend commented Jan 11, 2016

Copy link
Copy Markdown
Contributor

Hi all, do you have plans on merging this PR into master?

@agend

agend commented Jan 13, 2016

Copy link
Copy Markdown
Contributor

@abudnik @shaitan Do you have plan to merge this? We really need this feature) May be we can help in some way?

@abudnik

abudnik commented Jan 13, 2016

Copy link
Copy Markdown
Member Author

@agend You can merge both PR #679 and #681 into your local branch and test it on your test data. It would be very helpful for us.

@agend

agend commented Jan 13, 2016

Copy link
Copy Markdown
Contributor

@abudnik Is it save for data?

@shaitan

shaitan commented Jan 13, 2016

Copy link
Copy Markdown
Member

@agend It successfully passes our recovery tests but I wouldn't recommend you to test it on your production cluster until it is not tested on variety of test dataset or at least you have tested it on your test dataset that are close to your production.

PRs #679 and #681 are completed but before merging them we should fully test it to be sure that it is safe. Also we have one open question under discussion which doesn't affect to recovery correctness but defines recovery behaviour in case of error: "On which errors should copy-iterator stop?".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I saw these fields in #668 pull request, why do you update them in two different places? Is this the same code as in #668 ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've fixed distinction between _update_stats() code in both PRs (509bd3b). They are the same now.

@bioothod

Copy link
Copy Markdown
Member

Should I merge this pull request?

@shaitan

shaitan commented May 23, 2016

Copy link
Copy Markdown
Member

I will overlook and actualize it later today.

@shaitan

shaitan commented May 24, 2016

Copy link
Copy Markdown
Member

It depends on #681 which has conflicts with current master.

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.

5 participants