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

Make the foreach early-exit semantics consistent #1222

Merged
merged 6 commits into from
May 28, 2020

Conversation

eddyashton
Copy link
Member

Noticed as part of #1221 (and one need manually merging when the other goes in, due to similar changes in tx_view.h):

TxView::foreach takes a visitor lambda, and returning false is supposed to mean "I'm done, don't call me again". We need to run this twice, for both the original state and the current write set. The early-out-on-false behaviour only applied to the writes - if you decided you were done after looking at something in the state, you still get called with something in the write set. This fixes that - return false means you won't get called again at all.

On a related point, foreach itself returned a bool with no meaning. It could return "the lambda returned false at some point", or "you saw everything", but none of these seem particularly clear or useful. For clarity, I've just removed the return type entirely.

++unit tests

@eddyashton eddyashton requested a review from a team as a code owner May 27, 2020 15:37
@ghost
Copy link

ghost commented May 27, 2020

foreach_tests@8766 aka 20200528.6 vs master ewma over 50 builds from 8553 to 8761
images

Copy link
Member

@achamayou achamayou left a comment

Choose a reason for hiding this comment

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

Nice catch!

src/kv/test/kv_test.cpp Outdated Show resolved Hide resolved
src/kv/tx_view.h Outdated Show resolved Hide resolved
@eddyashton eddyashton merged commit 1d517bd into microsoft:master May 28, 2020
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.

3 participants