-
Notifications
You must be signed in to change notification settings - Fork 600
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: the signature unmatch in async trait function #3068
Conversation
79243aa
to
a78a9cc
Compare
a78a9cc
to
ad9ec87
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.
let mut mem_table_iter = mem_table_iter
.map(|(k, v)| Ok::<_, StorageError>((k, v)))
.peekable();
Using the original impl can help implement this with async_stream. As we are not using async_stream for now, I'd be okay with this change.
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.
Should wait for #3057 to be merged first?
@@ -422,7 +418,7 @@ impl<S: StateStore> StateTableRowIter<S> { | |||
(Some(_), Some(_)) => { | |||
// Throw the error. | |||
cell_based_table_iter.next().await.unwrap()?; | |||
mem_table_iter.next().unwrap()?; | |||
mem_table_iter.next().unwrap(); |
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.
We may remove this line since there's no error to throw for memtable iterator now. :)
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.
I think it's seperate problem so I submit another PR. #3071
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.
Emmm think for twice and I think you are right.. It should be done in this PR. anyway..
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.
Actually previous it also can not be Error. It's always map with Ok.
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.
Previously there’re some decoding stuff in the map
body so it can be error. Seems this has changed.😁
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 PR also works in my case!
Good Job!
Codecov Report
@@ Coverage Diff @@
## main #3068 +/- ##
==========================================
- Coverage 73.52% 73.52% -0.01%
==========================================
Files 732 732
Lines 99433 99431 -2
==========================================
- Hits 73106 73103 -3
- Misses 26327 26328 +1
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 |
What's changed and what's your intention?
When we use
state_table.iter()
in a async trait function, it shows a confusing compiler error about signature mismatch. I think it's compiler's problem but I do not get too many evidence. I just removed the unnecessary closure mentioned in the error and it seems work.@wcy-fdu can you please check that it fixs your problem? I have tested it on my version.
The compiler error looks like:
Because the
.map
looks unnecessary, so I just removed it.Checklist
./risedev check
(or alias,./risedev c
)Refer to a related PR or issue link (optional)