Consolidate query killing integration tests into single class#18300
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18300 +/- ##
============================================
- Coverage 63.65% 63.63% -0.03%
Complexity 1659 1659
============================================
Files 3244 3246 +2
Lines 197390 197512 +122
Branches 30555 30578 +23
============================================
+ Hits 125646 125677 +31
- Misses 61690 61800 +110
+ Partials 10054 10035 -19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
noob-se7en
added
testing
| @Test(priority = 1) | ||
| public void testCpuBasedBrokerQueryKilling() | ||
| throws Exception { | ||
| updateClusterConfig(Map.of( |
There was a problem hiding this comment.
Are all these cluster configs being updated for each test behind PinotClusterConfigChangeListener?
As prev test might be written because of components requiring restarts for configs to kick in.
There was a problem hiding this comment.
The WatcherTask inner class implements PinotClusterConfigChangeListener, which makes these dynamic?
| Accounting.BROKER_PREFIX + "." + Accounting.Keys.CRITICAL_LEVEL_HEAP_USAGE_RATIO, "1.1", | ||
| Accounting.BROKER_PREFIX + "." + Accounting.Keys.PANIC_LEVEL_HEAP_USAGE_RATIO, "1.1" | ||
| )); | ||
| Thread.sleep(5000); |
There was a problem hiding this comment.
This eliminates the purpose of reducing the test time. We should do a timed check of QueryMonitorConfig
There was a problem hiding this comment.
Good call, done.
mayankshriv
force-pushed
the
integ-tests-cleanup
branch
from
2ddbff1 to
5b9b277
Compare
| // Helpers | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| private void resetAccountingConfigs() |
There was a problem hiding this comment.
This slows down the test. Given we control the order of the test, let's not reset the config, but only moving forward
- Merge CPU broker, CPU server, and OOM server killing tests into one shared cluster - Use forward-only config progression (no full reset between tests) - Add testDefaultValues to verify accountant defaults after startup - Add testDynamicallyToggleQueryKill to verify dynamic config toggling - Use dependsOnMethods for explicit test ordering and skip-on-failure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mayankshriv
force-pushed
the
integ-tests-cleanup
branch
from
5b9b277 to
a713ab2
Compare
4 participants
Summary
CpuBasedBrokerQueryKillingIntegrationTest,CpuBasedServerQueryKillingIntegrationTest,MemoryBasedServerQueryKillingIntegrationTest) and their abstract base into one concreteQueryKillingIntegrationTestupdateClusterConfig(), runs assertions, then resets configs in afinallyblockNet result: -115 lines, 3 cluster starts → 1 cluster start (~1-3 min wall-clock savings)
Test plan
./mvnw -pl pinot-integration-tests -Dtest=QueryKillingIntegrationTest test— all 5 tests pass (resource stats, CPU broker kill, CPU server kill, memory server kill, resource stats after killing)testResourceUsageStatsAfterKillingverifies the accountant is not degraded after OOM kills🤖 Generated with Claude Code