(improvement) unit tests for benchmarking query planning. - #653
Conversation
b9a8faa to
a797262
Compare
|
Latest commit add Host policy. Now it looks like this: |
|
@mykaul , thanks looks great, can you implement them in pytest and mark with |
a797262 to
d850f81
Compare
There is no pytest.ini, so I've added one. The tests were already in pytest, now they are in the right dir, in a follow-up commit. |
|
There is |
dkropachev
left a comment
dkropachev
left a comment
There was a problem hiding this comment.
I love it, have some comments though.
Also can you please take a look at pytest-benchmark, it looks like exactly what we need.
| connections = [c for holders in cluster.get_connection_holders() for c in holders.get_connections()] | ||
|
|
||
| # make sure requests were sent on all connections | ||
| # Note: connections can be replaced in shard-aware environments, so we skip |
There was a problem hiding this comment.
Could you please create a separate pr for this change.
There was a problem hiding this comment.
I was going to, first wanted to see that it solves the issue - but it doesn't:
FAILED tests/integration/standard/test_cluster.py::ClusterTests::test_idle_heartbeat - assert not True
+ where True = any(<generator object ClusterTests.test_idle_heartbeat.<locals>.<genexpr> at 0x7f63c0292d40>)
I feel like I should git bisect it - if I can reproduce this locally first.
There was a problem hiding this comment.
Of course, it passes locally. Over and over, consistently. Not a single failure.
There was a problem hiding this comment.
since other PR doesn't break like that, would assume it's the new benchmark tests which are running together with all of the other unittests
There was a problem hiding this comment.
They all do - #686 as an example. So either I managed to sneak in something that fails everywhere, or something else is going on. I tend to blame me, but I have no proof yet, and it's very annoying it doesn't reproduce on my laptop.
There was a problem hiding this comment.
The tests/integration/standard/test_cluster.py change this thread is anchored on is no longer part of this PR's diff at all (dropped in a later push), so the original ask to split it into a separate PR is moot for this PR as it stands today.
Re: the test_idle_heartbeat flakiness discussed below — I confirmed the test is still present and unmodified on current master. Per @fruch's and @mykaul's discussion, this reads as a pre-existing, likely repo-wide intermittent failure (also observed on #686) rather than something specific to this PR, and it wasn't reproducible locally even by the PR author. I'm treating further bisection/root-causing of that flakiness as out of scope for fixing up this PR; leaving this thread open in case a maintainer wants to track it separately.
8c75060 to
76b79c3
Compare
76b79c3 to
6760c35
Compare
|
gevent issue? |
Add micro-benchmarks measuring query plan generation throughput for DCAwareRoundRobinPolicy, RackAwareRoundRobinPolicy, TokenAwarePolicy, DefaultLoadBalancingPolicy, and HostFilterPolicy. Uses pytest-benchmark for accurate timing and statistical reporting with a simulated 45-node cluster topology (5 DCs x 3 racks x 3 nodes) and 100,000 deterministic queries. The mock cluster's token-range/replica map (vnode_cluster) and the generated query load (benchmark_queries) are independent, module-scoped fixtures sized via separate TOKEN_RANGE_COUNT / QUERY_COUNT constants, so either can be tuned without touching the other. Also rename tests/integration/standard/column_encryption/test_policies.py to test_encrypted_policies.py to avoid module name conflicts when running the full test suite. Run with: pytest -m benchmark tests/performance/ Benchmark results comparing master vs PR scylladb#651 optimizations (Python 3.14.3, pytest-benchmark 5.2.3, GC disabled, median): Policy | master (Kops/s) | PR#651 (Kops/s) | Speedup --------------------------|-----------------|-----------------|-------- DCAware | 833 | 1898 | 2.3x RackAware | 542 | 1589 | 2.9x TokenAware(DCAware) | 135 | 572 | 4.2x TokenAware(RackAware) | 123 | 539 | 4.4x Default(DCAware) | 674 | 1257 | 1.9x HostFilter(DCAware) | 394 | 579 | 1.5x
6760c35 to
04ad714
Compare
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Comment |
|
Rebased onto current
Testing done:
CI: Rebase triggered a fresh run. Previously-failing Pushed the fix amended into the existing single commit (no new commits) to |
There was a problem hiding this comment.
Pull request overview
Adds pytest-based micro-benchmarks for load-balancing query-plan generation and supporting benchmark configuration.
Changes:
- Adds benchmarks for DC-aware, rack-aware, token-aware, default, and host-filter policies.
- Registers the benchmark marker and documents/suppresses saved benchmark results.
- Adds an unrelated, skipped column-encryption integration test suite.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/performance/test_policy_performance.py |
Adds simulated-cluster policy benchmarks. |
tests/performance/conftest.py |
Documents benchmark result comparison. |
tests/integration/standard/column_encryption/test_encrypted_policies.py |
Adds skipped column-encryption integration tests. |
pyproject.toml |
Registers the pytest benchmark marker. |
.gitignore |
Ignores saved benchmark output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| h = Host(MockEndPoint(address), SimpleConvictionPolicy, host_id=h_id) | ||
| h.set_location_info(dc, rack) |
| @pytest.mark.benchmark | ||
| def test_dc_aware(benchmark, vnode_cluster, benchmark_queries): |
test_idle_heartbeat snapshots each connection's request_ids keyed by id(connection), sleeps for a couple of heartbeat intervals, then looks the connections back up by id() to validate the heartbeats. If a connection object is replaced in between, the lookup raises KeyError. master already fixed the main source of this: wait_for_all_pools() only waits for the first connection per host, so shard-aware connections to the remaining shards could still be opening (and replacing placeholders) during the sleep window. 454f727 and f348637 added _wait_for_all_shard_connections(), called before the snapshot is taken, and deliberately dropped an earlier "skip unknown connections" band-aid so the assertions stay meaningful once the pool is known to be stable. That fix only guarantees connection *count* has stabilized, though. It doesn't rule out an existing connection being swapped for a new object in the same shard slot while the count stays constant: - HostConnection._open_connection_to_missing_shard() replaces a shard's connection once orphaned_threshold_reached, independent of pool size. - HostConnection.return_connection() -> _replace() does the same when a connection is found defunct or closed. Both can fire at any time, including during this test's own sleep, and are more likely to fire under CPU/IO contention (e.g. other tests running concurrently delaying heartbeat responses) - which lines up with the "why does this pop now, and only intermittently" puzzle from the scylladb#653 review discussion (with @dkropachev and @fruch) about this same test's flakiness. Guard against the resulting KeyError by filtering to connections that are still present in the snapshot, but without silently weakening the test: in the common case (no replacement) the filtered list is identical to the full connection list, so behavior is unchanged from master. Only when the filter actually drops something do we log a warning (so a recurrence remains visible in CI) and we still hard-fail if every single connection was replaced, since at that point nothing was validated. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
test_idle_heartbeat snapshots each connection's request_ids keyed by id(connection), sleeps for a couple of heartbeat intervals, then looks the connections back up by id() to validate the heartbeats. If a connection object is replaced in between, the lookup raises KeyError. master already fixed the main source of this: wait_for_all_pools() only waits for the first connection per host, so shard-aware connections to the remaining shards could still be opening (and replacing placeholders) during the sleep window. 454f727 and f348637 added _wait_for_all_shard_connections(), called before the snapshot is taken, and deliberately dropped an earlier "skip unknown connections" band-aid so the assertions stay meaningful once the pool is known to be stable. That fix only guarantees connection *count* has stabilized, though. It doesn't rule out an existing connection being swapped for a new object in the same shard slot while the count stays constant: - HostConnection._open_connection_to_missing_shard() replaces a shard's connection once orphaned_threshold_reached, independent of pool size. - HostConnection.return_connection() -> _replace() does the same when a connection is found defunct or closed. Both can fire at any time, including during this test's own sleep, and are more likely to fire under CPU/IO contention (e.g. other tests running concurrently delaying heartbeat responses) - which lines up with the "why does this pop now, and only intermittently" puzzle from the scylladb#653 review discussion (with @dkropachev and @fruch) about this same test's flakiness. Guard against the resulting KeyError by filtering to connections that are still present in the snapshot, but without silently weakening the test: in the common case (no replacement) the filtered list is identical to the full connection list, so behavior is unchanged from master. Only when the filter actually drops something do we log a warning (so a recurrence remains visible in CI) and we still hard-fail if every single connection was replaced, since at that point nothing was validated. Compare both directions of the snapshot/current intersection, not just "every current connection is known". return_connection() has a genuine asynchronous gap: it pops a defunct/closed connection out of the pool's dict synchronously, but the replacement is inserted later by _replace(), which runs on another thread via session.submit(). A snapshot taken during that gap sees fewer current connections than were recorded, and every one of them is still "known" (nothing new has landed yet) - so "all current connections are known" is trivially true even though fewer connections than snapshotted are being validated. Tracking missing-from-current (snapshotted connections no longer present) in addition to unknown-in-current (new connections not in the snapshot) makes that partial-removal case surface as a warning too, per review feedback from @copilot-pull-request-reviewer. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
Not a very scientific one, but reasonable to get some measurements in terms of how different optimizations work. Example run (on #650 branch):
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.