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

During backup, collapse split posting lists into a single list. #4682

Merged
merged 10 commits into from
Feb 10, 2020

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Jan 28, 2020

Currently, the splits are preserved in a backup, this changes the
output of backup to only write a single key per posting list.


This change is Reviewable

Docs Preview: Dgraph Preview

Currently, the splits are preserved in a backup, this changes the
output of backup to only write a single key per posting list.
@martinmr martinmr requested review from manishrjain and a team as code owners January 28, 2020 00:45
Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

Why is this needed? The list would be split again when the database is running (and splits are enabled).

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @manishrjain)

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

This makes backup more robust. If the split logic changes, backups would still work across versions. It's fine if they end up being split again after the restore.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @manishrjain)

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Changed SingleListRollup to re-use l.rollup. l.rollup now gets passed a boolean controlling whether the split should be done.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @danielmai and @manishrjain)

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Please add tests to ensure that for a split posting list, this works as expected. Picks up all the data.

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @danielmai)

Splits have been re-enabled in version 1.2.1 after Jepsen tests
encoutered no errors.

Re-enabling splits in master.
Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Added tests.

Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @danielmai and @manishrjain)

@martinmr martinmr merged commit 2ad482a into master Feb 10, 2020
@martinmr martinmr deleted the martinmr/single-list-backups branch February 10, 2020 21:44
martinmr added a commit that referenced this pull request Jun 12, 2020
Currently, the splits are preserved in a backup, this changes the
output of backup to only write a single key per posting list.
martinmr added a commit that referenced this pull request Jul 9, 2020
Currently, the splits are preserved in a backup, this changes the
output of backup to only write a single key per posting list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants