-
Notifications
You must be signed in to change notification settings - Fork 526
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
fix: no need to expect mem table return Err #3071
Conversation
da6258d
to
6732f6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would cause wrong result. We only peeked memtable without calling .next.
Better to rewrite this part with async_stream to make everything clear.
Actually it should return Error at (state_table.next()). If not(I cant imagine why), it will directly panic. So I guess It will not make the code malfunction. |
Codecov Report
@@ Coverage Diff @@
## main #3071 +/- ##
==========================================
- Coverage 73.62% 73.52% -0.10%
==========================================
Files 736 732 -4
Lines 101467 99431 -2036
==========================================
- Hits 74701 73106 -1595
+ Misses 26766 26325 -441
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
But indeed. This part logic looks strange |
The It may look strange because we cannot take the error out of the peeked reference. 😢 |
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
What's changed and what's your intention?
Ideas from:
#3068 (comment)
Memtable do not return Error, so no need to call
.next()?
.Checklist
./risedev check
(or alias,./risedev c
)Refer to a related PR or issue link (optional)