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

Retry for stale reads in KV CAS chaos test #4769

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mprimi
Copy link
Contributor

@mprimi mprimi commented Nov 8, 2023

Relax KV CAS test which was failing immediately upon reading a key with a revision which is known to be stale.
Instead, retry the read until a newer version is returned.

Signed-off-by: Marco Primi [email protected]

@mprimi mprimi requested a review from a team as a code owner November 8, 2023 19:07
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

Prefer to leave direct gets but you can use checkFor() helper to wait for a certain period of time for the get to be current.

@mprimi mprimi force-pushed the marco/chaos_test_kv_direct_get branch from 6ad58b4 to a743862 Compare November 8, 2023 23:13
@mprimi mprimi changed the title Turn off DirectGet for KV bucket in test Retry for stale reads in KV CAS chaos test Nov 8, 2023
@mprimi
Copy link
Contributor Author

mprimi commented Nov 8, 2023

@derekcollison updated to leave DirectGet on, be more tolerant of stale reads, and retry with checkFor.

(2 commits for easier review, squashable on merge)

@derekcollison derekcollison self-requested a review November 9, 2023 00:19
server/jetstream_chaos_test.go Outdated Show resolved Hide resolved
server/jetstream_chaos_test.go Outdated Show resolved Hide resolved
@mprimi mprimi force-pushed the marco/chaos_test_kv_direct_get branch from a743862 to 564661c Compare November 9, 2023 16:53
@mprimi
Copy link
Contributor Author

mprimi commented Nov 9, 2023

Updated indentation in 2nd commit
Added 3rd commit to switch to require_ where appropriate

@mprimi mprimi force-pushed the marco/chaos_test_kv_direct_get branch from 564661c to 88a10b1 Compare November 15, 2023 18:28
@mprimi
Copy link
Contributor Author

mprimi commented Nov 15, 2023

Just rebased, the Jenkins error comes from Run non JetStream/MQTT tests from the server package::TestProcessSignalMultipleProcessesGlob, the rest is green.

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

One comment on the allow direct update to false.

@mprimi mprimi force-pushed the marco/chaos_test_kv_direct_get branch from 88a10b1 to 136687e Compare November 17, 2023 23:53
@derekcollison
Copy link
Member

Can we check the failures, seems all tests fails so probably failed staticcheck or something.

Also, @mprimi is this the test that shows reset you wanted us to look at?

@mprimi
Copy link
Contributor Author

mprimi commented Dec 14, 2023

@derekcollison this got stale, I'll rebase.

The failing RAFT i mentioned is a different (draft) PR: #4626

When detecting a stale value, retry rather than fail immediately.

Signed-off-by: Marco Primi [email protected]
@mprimi
Copy link
Contributor Author

mprimi commented Dec 15, 2023

Freshly rebased on top of latest main.

@github-actions github-actions bot added the stale This issue has had no activity in a while label Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale This issue has had no activity in a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants