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.

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

feat: add iter->status() check for loop iterators #2395

Merged
merged 5 commits into from
Jul 7, 2024

Conversation

LindaSummer
Copy link
Contributor

@LindaSummer LindaSummer commented Jul 6, 2024

Issue

close #2200

Proposed Changes

  • add iterator status error handler after iteration.

Comment

  1. Some iterators checked Valid() after loop so I didn't add status validation.
    Here is rocksdb's document.

If there is no error, the status is Status::OK(). If the status is not OK, the iterator will be invalidated too. In another word, if Iterator::Valid() is true, status() is guaranteed to be OK() so it's safe to proceed other operations without checking status().

  1. Please correct me if I need add some more logic of error handler, or the error message I generated is not suitable.
    Thanks very much!

@LindaSummer LindaSummer changed the title enhance: add iter->status() check for loop iterators feat: add iter->status() check for loop iterators Jul 6, 2024
@LindaSummer LindaSummer force-pushed the feature/iterator_valid_check branch 2 times, most recently from 043e916 to f887cfc Compare July 6, 2024 04:13
@git-hulk
Copy link
Member

git-hulk commented Jul 6, 2024

LGTM, one comment inline.

@LindaSummer
Copy link
Contributor Author

LindaSummer commented Jul 7, 2024

Hi @git-hulk ,

I find it seems hard to create unit tests for the error handling code and pass sonarcloud coverage.
Could you give me some suggestions?

Best Regards,
Edward

@git-hulk
Copy link
Member

git-hulk commented Jul 7, 2024

@LindaSummer That’s fine for this PR, no need to add the extra tests.

Copy link

sonarcloud bot commented Jul 7, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
33.7% Coverage on New Code (required ≥ 50%)

See analysis details on SonarCloud

@LindaSummer
Copy link
Contributor Author

Hi @git-hulk ,

Got it! 😊
Thanks very much for your help and patience!

Best Regards,
Edward

@git-hulk git-hulk merged commit 51569ad into apache:unstable Jul 7, 2024
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check iterator status after quit loop
3 participants