backend/local: check and return iter.Error when pebble is not valid #497
Conversation
|
LGTM |
|
/test |
|
/run-all-tests |
1 similar comment
|
/run-all-tests |
|
|
||
| if !iter.First() { | ||
| if !iter.Valid() { |
There was a problem hiding this comment.
What about iter.Last() at line 516?
| @@ -851,6 +863,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") | |||
| } | |||
There was a problem hiding this comment.
What about iter.Last() at line 877?
There was a problem hiding this comment.
What about this comment? @glorv
There was a problem hiding this comment.
do we need to check Valid() after iter.Last(), as we did for iter.First()?
There was a problem hiding this comment.
yes, see #880-883 @lance6716
There was a problem hiding this comment.
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 🤔 )
There was a problem hiding this comment.
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.
|
/run-all-tests |
|
@glorv Please make sure this PR adds all necessary error checks for pebble usage. |
I have checked all the pebble iterators, there should be no remain. |
|
LGTM |
lance6716
added
status/LGT2
What problem does this PR solve?
Check pebble iter.Error when iter is not valid so if iter returns an error (such as caused by exceeding max open file limit), lightning won't omit the actual error
What is changed and how it works?
Check List
Tests
Side effects
Related changes