Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

backend/local: check and return iter.Error when pebble is not valid #497

Merged
merged 8 commits into from
Dec 8, 2020
Merged

backend/local: check and return iter.Error when pebble is not valid #497

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
61d6382
check and return iter.Error when pebble is not valid
glorv Nov 30, 2020
b470eb9
replace errors.Annotatef with errors.Annotate
glorv Nov 30, 2020
c23ae71
fix
glorv Dec 1, 2020
d488a0b
Merge branch 'master' into iter-error
lance6716 Dec 1, 2020
aa6be9f
add error check for iter.Last()
glorv Dec 1, 2020
1d8b293
Merge branch 'iter-error' of https://github.com/pingcap/tidb-lightnin…
glorv Dec 1, 2020
11e4cc6
Merge branch 'master' into iter-error
glorv Dec 1, 2020
5cf42d6
Merge branch 'master' into iter-error
lance6716 Dec 8, 2020
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
35 changes: 28 additions & 7 deletions lightning/backend/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,8 +500,12 @@ func (local *local) WriteToTiKV(
opt := &pebble.IterOptions{LowerBound: regionRange.start, UpperBound: regionRange.end}
iter := engineFile.db.NewIter(opt)
defer iter.Close()
iter.First()
if iter.Error() != nil {
return nil, nil, errors.Annotate(iter.Error(), "failed to read the first key")
}

if !iter.First() {
if !iter.Valid() {
Copy link
Member

Choose a reason for hiding this comment

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

What about iter.Last() at line 516?

log.L().Info("keys within region is empty, skip ingest", zap.Binary("start", start),
zap.Binary("regionStart", region.Region.StartKey), zap.Binary("end", end),
zap.Binary("regionEnd", region.Region.EndKey))
Expand All @@ -510,6 +514,9 @@ func (local *local) WriteToTiKV(

firstKey := codec.EncodeBytes([]byte{}, iter.Key())
iter.Last()
if iter.Error() != nil {
return nil, nil, errors.Annotate(iter.Error(), "failed to seek to the last key")
}
lastKey := codec.EncodeBytes([]byte{}, iter.Key())

u := uuid.New()
Expand Down Expand Up @@ -596,6 +603,10 @@ func (local *local) WriteToTiKV(
}
}

if iter.Error() != nil {
return nil, nil, errors.Trace(iter.Error())
}

if count > 0 {
for i := range clients {
requests[i].Chunk.(*sst.WriteRequest_Batch).Batch.Pairs = pairs[:count]
Expand All @@ -605,10 +616,6 @@ func (local *local) WriteToTiKV(
}
}

if iter.Error() != nil {
return nil, nil, errors.Trace(iter.Error())
}

var leaderPeerMetas []*sst.SSTMeta
for i, wStream := range clients {
if resp, closeErr := wStream.CloseAndRecv(); closeErr != nil {
Expand Down Expand Up @@ -705,16 +712,24 @@ func (local *local) readAndSplitIntoRange(engineFile *LocalFile) ([]Range, error
iter := engineFile.db.NewIter(nil)
defer iter.Close()

iterError := func(e string) error {
err := iter.Error()
if err != nil {
return errors.Annotate(err, e)
}
return errors.New(e)
}

var firstKey, lastKey []byte
if iter.First() {
firstKey = append([]byte{}, iter.Key()...)
} else {
return nil, errors.New("could not find first pair, this shouldn't happen")
return nil, iterError("could not find first pair")
}
if iter.Last() {
lastKey = append([]byte{}, iter.Key()...)
} else {
return nil, errors.New("could not find last pair, this shouldn't happen")
return nil, iterError("could not find last pair")
}
endKey := nextKey(lastKey)

Expand Down Expand Up @@ -851,6 +866,9 @@ func (local *local) writeAndIngestByRange(
defer iter.Close()
// Needs seek to first because NewIter returns an iterator that is unpositioned
hasKey := iter.First()
if iter.Error() != nil {
return errors.Annotate(iter.Error(), "failed to read the first key")
}
Copy link
Member

Choose a reason for hiding this comment

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

What about iter.Last() at line 877?

Copy link
Member

Choose a reason for hiding this comment

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

What about this comment? @glorv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to check Valid() after iter.Last(), as we did for iter.First()?

Copy link
Contributor Author

@glorv glorv Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yes, see #880-883 @lance6716

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean iter.Error() here could cover iter.Valid()? For iter.First() why we need them both (maybe iterator will keep valid after normal operation on iterator 🤔 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we only need to check iter.Error() != nil which indicates that something goes wrong. iter.Error() != nil always means iter.Valid() is false. if iter.Error() == nil and iter.Valid() is false, this means the iterator iters to the end, this is ok to our logic.

if !hasKey {
log.L().Info("There is no pairs in iterator",
zap.Binary("start", start),
Expand All @@ -860,6 +878,9 @@ func (local *local) writeAndIngestByRange(
}
pairStart := append([]byte{}, iter.Key()...)
iter.Last()
if iter.Error() != nil {
return errors.Annotate(iter.Error(), "failed to seek to the last key")
}
pairEnd := append([]byte{}, iter.Key()...)

var regions []*split.RegionInfo
Expand Down