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

Introduce backup data formats for cross-version compatibility. #3575

Merged
merged 4 commits into from
Jun 24, 2019

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Jun 18, 2019

Currently backups are storing data in the internal format. If that
format changes backups will not work accross version. This PR introduces
new formats for both keys and values (posting lists) to solve this
problem.


This change is Reviewable

martinmr added 2 commits June 18, 2019 15:52
currently backups are storing data in the internal format. If that
format changes backups will not work accross version. This PR introduces
new formats for both keys and values (posting lists) to solve this
problem.
@martinmr martinmr requested review from manishrjain and a team as code owners June 18, 2019 22:55
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.

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @martinmr)


protos/pb.proto, line 504 at r1 (raw file):

// A key stored in the format used for writing backups.
message BackupKey {
  bytes byteType = 1;

Make this an enum.


protos/pb.proto, line 510 at r1 (raw file):

  string term = 5;
  uint32 count = 6;
  bytes byte_prefix = 7;

We shouldn't need this. We just need enough info here to call the keys.go functions to generate the corresponding key.


x/keys.go, line 372 at r1 (raw file):

// FromBackupKey takes a key in the format used for backups and converts it to a ParsedKey.
func FromBackupKey(key *pb.BackupKey) (*ParsedKey, error) {

This shouldn't return a ParsedKey. It should just return a key, which uses DataKey, CountKey, etc. functions to generate the right key.

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.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain)


protos/pb.proto, line 504 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Make this an enum.

Done.


protos/pb.proto, line 510 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

We shouldn't need this. We just need enough info here to call the keys.go functions to generate the corresponding key.

Done.


x/keys.go, line 372 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This shouldn't return a ParsedKey. It should just return a key, which uses DataKey, CountKey, etc. functions to generate the right key.

Done.

@martinmr martinmr requested a review from manishrjain June 19, 2019 20:00
@martinmr martinmr dismissed manishrjain’s stale review June 19, 2019 20:01

addressed comments

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: Nice and sweet change!

Reviewed 5 of 5 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @martinmr)


x/keys.go, line 244 at r2 (raw file):

// IsCountOrCountRev returns whether the key is a count or a count rev key.
func (p ParsedKey) IsCountOrCountRev() bool {
	return p.bytePrefix == DefaultPrefix && (p.byteType == ByteCount ||

Don't repeat logic.

return p.IsCount() || p.IsCountRev

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.

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @manishrjain)


x/keys.go, line 244 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Don't repeat logic.

return p.IsCount() || p.IsCountRev

Done.

@martinmr martinmr merged commit 0a1fae2 into master Jun 24, 2019
@martinmr martinmr deleted the martinmr/backup-data-format branch June 24, 2019 22:00
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
…h-io#3575)

Currently backups are storing data in the internal format. If that
format changes backups will not work accross version. This PR introduces
new formats for both keys and values (posting lists) to solve this
problem.
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.

2 participants