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.

Sign up for GitHub

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

Fixes the toBackupList function by removing the loop #3869

Merged
merged 2 commits into from
Aug 26, 2019
Merged

Fixes the toBackupList function by removing the loop #3869

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dbeef9f
Fixes the toBackupList function by removing the loop
Aug 26, 2019
a651ab9
Removed one if condition check which should always be true
Aug 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 46 additions & 67 deletions ee/backup/backup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
package backup

import (
"bytes"
"compress/gzip"
"context"
"encoding/binary"
Expand Down Expand Up @@ -198,86 +197,66 @@ func (m *Manifest) GoString() string {
func (pr *Processor) toBackupList(key []byte, itr *badger.Iterator) (*bpb.KVList, error) {
list := &bpb.KVList{}

// TODO: This for loop doesn't seem right. We should only have one iteration here.
for itr.Valid() {
item := itr.Item()
if !bytes.Equal(item.Key(), key) {
return list, nil
item := itr.Item()
if item.Version() < pr.Request.SinceTs || item.IsDeletedOrExpired() {
// Ignore versions less than given timestamp, or skip older versions of
// the given key by returning an empty list.
return list, nil
}

switch item.UserMeta() {
case posting.BitEmptyPosting, posting.BitCompletePosting, posting.BitDeltaPosting:
l, err := posting.ReadPostingList(key, itr)
if err != nil {
return nil, errors.Wrapf(err, "while reading posting list")
}
if item.Version() < pr.Request.SinceTs {
// Ignore versions less than given timestamp, or skip older versions of
// the given key.
return list, nil
kvs, err := l.Rollup()
if err != nil {
return nil, errors.Wrapf(err, "while rolling up list")
}

switch item.UserMeta() {
case posting.BitEmptyPosting, posting.BitCompletePosting, posting.BitDeltaPosting:
l, err := posting.ReadPostingList(key, itr)
if err != nil {
return nil, errors.Wrapf(err, "while reading posting list")
}
kvs, err := l.Rollup()
for _, kv := range kvs {
backupKey, err := toBackupKey(kv.Key)
if err != nil {
return nil, errors.Wrapf(err, "while rolling up list")
}

for _, kv := range kvs {
backupKey, err := toBackupKey(kv.Key)
if err != nil {
return nil, err
}
kv.Key = backupKey

backupPl, err := toBackupPostingList(kv.Value)
if err != nil {
return nil, err
}
kv.Value = backupPl
}
list.Kv = append(list.Kv, kvs...)

case posting.BitSchemaPosting:
var valCopy []byte
if !item.IsDeletedOrExpired() {
// No need to copy value if item is deleted or expired.
var err error
valCopy, err = item.ValueCopy(nil)
if err != nil {
return nil, errors.Wrapf(err, "while copying value")
}
return nil, err
}
kv.Key = backupKey

backupKey, err := toBackupKey(key)
backupPl, err := toBackupPostingList(kv.Value)
if err != nil {
return nil, err
}

kv := &bpb.KV{
Key: backupKey,
Value: valCopy,
UserMeta: []byte{item.UserMeta()},
Version: item.Version(),
ExpiresAt: item.ExpiresAt(),
}
list.Kv = append(list.Kv, kv)

if item.DiscardEarlierVersions() || item.IsDeletedOrExpired() {
return list, nil
kv.Value = backupPl
}
list.Kv = append(list.Kv, kvs...)
case posting.BitSchemaPosting:
var valCopy []byte
if !item.IsDeletedOrExpired() {
Copy link

Choose a reason for hiding this comment

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

It seems like this case may always be hit now that you have a check at the top for:

if item.Version() < pr.Request.SinceTs || item.IsDeletedOrExpired() {
    return list, nil
}

Wondering if this case can be simplified from this or if that initial if statement needs to be adjusted for this case?

// No need to copy value if item is deleted or expired.
var err error
valCopy, err = item.ValueCopy(nil)
if err != nil {
return nil, errors.Wrapf(err, "while copying value")
}
}

// Manually advance the iterator. This cannot be done in the for
// statement because ReadPostingList advances the iterator so this
// only needs to be done for BitSchemaPosting entries.
itr.Next()
backupKey, err := toBackupKey(key)
if err != nil {
return nil, err
}

default:
return nil, errors.Errorf(
"Unexpected meta: %d for key: %s", item.UserMeta(), hex.Dump(key))
kv := &bpb.KV{
Key: backupKey,
Value: valCopy,
UserMeta: []byte{item.UserMeta()},
Version: item.Version(),
ExpiresAt: item.ExpiresAt(),
}
list.Kv = append(list.Kv, kv)
default:
return nil, errors.Errorf(
"Unexpected meta: %d for key: %s", item.UserMeta(), hex.Dump(key))
}

// This shouldn't be reached but it's being added here because the golang
// compiler complains about the missing return statement.
return list, nil
}

Expand Down