Skip to content

fix(tests): resolve flaky data mismatch in kvrocks2redis consistency check - #3647

Closed
rishi919-rgb wants to merge 1 commit into
apache:unstablefrom
rishi919-rgb:fix-kvrocks2redis-consistency-flakiness
Closed

rishi919-rgb wants to merge 1 commit into
apache:unstablefrom
rishi919-rgb:fix-kvrocks2redis-consistency-flakiness

Conversation

@rishi919-rgb

Copy link
Copy Markdown

Fixes #3153.
Refs #3516.

Motivation

In CI test runs for kvrocks2redis, check_consistency.py periodically fails with data mismatches such as:

AssertionError: Data mismatch for key 'key_49': source data: 'value_49' destination data: 'None'

Because kvrocks2redis asynchronously replays WAL batches to destination Redis, under heavily loaded CI runners, replication may take slightly longer than the previous 3 attempts (totaling ~0.3s). When this threshold is exceeded before the WAL entry is processed, dst_data remains None (or stale), causing false-positive CI failures.

Additionally:

  • In compare_redis_data(key_file), line 89 previously raised AssertionError(f"Data mismatch for key '{key}'...") where key was undefined, resulting in a NameError: name 'key' is not defined.
  • key_file comparison had no retry mechanism.
  • user_key.log in populate-kvrocks.py was never flushed or closed.

Solution

  • Introduced _wait_and_compare helper in check_consistency.py that polls up to 30 attempts at 100ms intervals (up to 3.0s total timeout). It returns immediately as soon as src_data == dst_data, so passing tests incur zero unnecessary delay while accommodating CI scheduling spikes.
  • Used _wait_and_compare in both _import_and_compare and compare_redis_data(key_file).
  • Fixed the NameError in compare_redis_data by referencing keys[0].
  • Ensured user_key.log in populate-kvrocks.py is flushed after writes and closed on script exit.

@github-actions

Copy link
Copy Markdown

Hi @rishi919-rgb,

Thank you for your pull request. Please review our Contributing Guide.

Please make sure you understand your changes and explain your reasoning in this pull request. Low-quality pull requests may be closed.

@jihuayu

jihuayu commented Sep 26, 2026

Copy link
Copy Markdown
Member

Hi @rishi919-rgb Please read the Contributing Guide and follow our AI usage guidelines.

Thanks.

@rishi919-rgb

Copy link
Copy Markdown
Author

Hi @jihuayu,

Thanks for the reminder! I've reviewed the Contributing Guide thoroughly and am happy to clarify the rationale and details of this patch.

I investigated issue #3153 reported by @git-hulk regarding the intermittent AssertionError: Data mismatch for key 'key_49' in the kvrocks2redis consistency tests.

Here is what was identified and addressed:

  1. Replication timing flakiness in CI: In _import_and_compare, writes are submitted to Kvrocks and then checked against destination Redis. Because kvrocks2redis asynchronously tails the RocksDB WAL and pushes updates to Redis, loaded CI runners occasionally take longer than the previous 3-attempt limit (attempts <= 3 with 0.1s intervals, totaling ~300ms). When replication lags past 300ms, destination Redis still has None, triggering a false-positive test failure.
    • The fix unifies the retry logic in _wait_and_compare with a 3.0s ceiling (30 attempts at 100ms), but immediately returns as soon as src_data == dst_data. Passing tests incur zero added delay while giving loaded CI environments enough buffer to avoid flakiness.
  2. Hidden NameError in compare_redis_data: When inspecting compare_redis_data(key_file), line 89 previously raised AssertionError(f"Data mismatch for key '{key}'..."), but variable key was never bound in that scope (it was parsed into keys), which would trigger a NameError: name 'key' is not defined instead of reporting the mismatched key. This was fixed by referencing keys[0].
  3. Missing retry logic on key_file: compare_redis_data(key_file) previously checked keys instantaneously with 0 retry tolerance; it now also uses _wait_and_compare.
  4. Unflushed file in populate-kvrocks.py: Ensured user_key.log is flushed after writes and closed on exit via a finally block to avoid truncated log outputs.

All Python syntax checks and pre-commit workflows pass cleanly. Please let me know if you would like any adjustments to the timeout parameters or structure!

…check

- Introduce _wait_and_compare helper with adequate retry attempts (up to 3s) for asynchronous WAL replication to destination Redis
- Fix NameError in compare_redis_data where undefined key was referenced instead of keys[0]
- Apply retry logic to key_file comparison
- Ensure user_key.log in populate-kvrocks.py is flushed after writes and closed on exit
- Fixes apache#3153
@rishi919-rgb
rishi919-rgb force-pushed the fix-kvrocks2redis-consistency-flakiness branch from fc4256d to c452be8 Compare September 27, 2026 08:54
@PragmaTwice

Copy link
Copy Markdown
Member

I think you failed to clarify your AI usage and follow the AI policy. Closed.

@rishi919-rgb

rishi919-rgb commented Sep 27, 2026 •

Copy link
Copy Markdown
Author

Hi @PragmaTwice @jihuayu,

Apologies for the misunderstanding earlier — I did not intend to sidestep the policy. To clarify transparently in accordance with the Kvrocks AI Guidelines:

I used an AI coding assistant to help analyze the test failure logs in issue #3153 and draft the initial patch. Following that, I personally inspected the test code (check_consistency.py and populate-kvrocks.py), verified the logic behind the replication race condition, verified the NameError on keys[0], and tested the changes locally to make sure they adhere to Kvrocks standards.

I take full responsibility for this contribution and am committed to maintaining it and addressing any review feedback.

If acceptable, could we please reopen this PR for review? Thank you for your guidance and patience.

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.

Flaky test due to the data mismatch in kvrocks2redis consistency check

3 participants