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

safekeeper: check for non-consecutive writes in safekeeper.rs #8640

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

arssher
Copy link
Contributor

@arssher arssher commented Aug 7, 2024

wal_storage.rs already checks this, but since this is a quite legit scenario check it at safekeeper.rs (consensus level) as well.

ref #8212

wal_storage.rs already checks this, but since this is a quite legit scenario
check it at safekeeper.rs (consensus level) as well.

ref #8212
@arssher arssher requested a review from a team as a code owner August 7, 2024 16:30
@arssher arssher requested a review from jcsp August 7, 2024 16:30
@arssher
Copy link
Contributor Author

arssher commented Aug 7, 2024

There was an idea to try to differ when these conditions are 'expected' and not, but I don't see a useful way to do this. It is normal to see this in case of reconnections. Something different is hard to imagine and likely means serious bug, like wal truncation repeatedly truncating not at the point where streaming later starts; then we won't be able to write anything at all and also notice the problem.

Copy link

github-actions bot commented Aug 7, 2024

2112 tests run: 2043 passed, 0 failed, 69 skipped (full report)


Code coverage* (full report)

  • functions: 32.6% (7162 of 21950 functions)
  • lines: 50.5% (57867 of 114514 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
c10a29d at 2024-08-07T17:33:29.116Z :recycle:

@arssher arssher merged commit 4c5a0fd into main Aug 20, 2024
63 checks passed
@arssher arssher deleted the sk-check-non-consecutive-writes branch August 20, 2024 04:12
jcsp added a commit that referenced this pull request Aug 20, 2024
#8771)

Reverts #8640

This broke `test_last_log_term_switch` via a merge race of some kind.
arssher added a commit that referenced this pull request Aug 21, 2024
wal_storage.rs already checks this, but since this is a quite legit scenario
check it at safekeeper.rs (consensus level) as well.

ref #8212

This is a take 2; previous PR #8640 had been reverted because interplay
with another change broke test_last_log_term_switch.
arssher added a commit that referenced this pull request Sep 3, 2024
wal_storage.rs already checks this, but since this is a quite legit scenario
check it at safekeeper.rs (consensus level) as well.

ref #8212

This is a take 2; previous PR #8640 had been reverted because interplay
with another change broke test_last_log_term_switch.
arssher added a commit that referenced this pull request Sep 3, 2024
wal_storage.rs already checks this, but since this is a quite legit scenario
check it at safekeeper.rs (consensus level) as well.

ref #8212

This is a take 2; previous PR #8640 had been reverted because interplay
with another change broke test_last_log_term_switch.
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