Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition in PinotTableRestletResourceTest#testTableTasksCleanupWithNonActiveTasks that caused flaky test failures. The issue occurred when the minion task queue was resumed before table deletion completed, allowing concurrent task transitions to interfere with cleanup operations.
Changes:
- Move task queue resume logic from before table deletion to after deletion in a
finallyblock - Add explanatory comments describing why the queue must remain stopped during deletion
- Ensure queue resume happens even if deletion fails, preventing test interference
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17652 +/- ##
=========================================
Coverage 63.24% 63.24%
Complexity 1466 1466
=========================================
Files 3190 3190
Lines 191963 191963
Branches 29410 29410
=========================================
+ Hits 121400 121401 +1
+ Misses 61059 61052 -7
- Partials 9504 9510 +6
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:
|
xiangfu0
changed the title
xiangfu0
force-pushed
the
codex/fix-table-tasks-cleanup-flaky
branch
from
3d37641 to
e3b021a
Compare
|
Addressed another flake from CI in Validation:
|
| waitForTaskState(taskName, TaskState.IN_PROGRESS); | ||
|
|
||
| // stop the task queue to abort the task | ||
| // Stop the task queue to abort the task. Keep it stopped until table deletion is complete to avoid |
There was a problem hiding this comment.
Is this test capturing a race condition?
There was a problem hiding this comment.
^^ Is this capturing a valid race condition in production code?
| } else { | ||
| truncated = 0.0d; | ||
| } | ||
| // Normalize -0.0 to +0.0 for deterministic output and test stability. |
There was a problem hiding this comment.
Is this needed? This is an actual production fix, suggest putting it in a separate PR
Extracted from apache#17652 as a standalone production fix. - Use allocation-free math truncation in no-scale path - Preserve NaN/+Inf/-Inf behavior for no-scale truncate - Normalize -0.0 to canonical +0.0 for deterministic output - Add regression coverage for signed-zero bit pattern and non-finite values
|
Split out the production truncate(value) fix into a dedicated PR: #17677. That PR includes the non-finite handling guard, allocation-free no-scale truncation path, signed-zero normalization, and expanded unit coverage. |
Keep apache#17652 focused on flaky-test fixes only; production truncate(value) fix is now tracked in apache#17677.
|
Updated this PR to remove all net pinot-core truncate changes; it now only contains flaky-test fixes in controller/integration tests. The production truncate(value) fix remains in dedicated PR #17677. Validation rerun:
|
xiangfu0
changed the title
xiangfu0
force-pushed
the
codex/fix-table-tasks-cleanup-flaky
branch
from
7e9c12f to
d2fcb1e
Compare
xiangfu0
added
testing
Problem
Two flaky test failures were observed in CI:
PinotTableRestletResourceTest#testTableTasksCleanupWithNonActiveTasksFailed to delete job ... from queue ...during table deletion.TruncateDecimalTransformFunctionTest#testTruncateDecimalTransformFunctionexpected [0.0] but found [-0.0].Changes
1) Controller test race fix
testTableTasksCleanupWithNonActiveTasks, keep the minion task queue stopped while table deletion runs.finallyblock to avoid leaking queue state to other tests.2) Truncate signed-zero fix
TruncateDecimalTransformFunction, changed the single-argument path (truncate(value)) to use the same truncation logic astruncate(value, 0)viaBigDecimal.setScale(0, RoundingMode.DOWN).TruncateDecimalTransformFunctionTestfortruncate(-0.4)to guarantee canonical0.0output (not-0.0).Why this fixes flakiness
truncate(value)withtruncate(value, 0)semantics.Validation
./mvnw -pl pinot-controller -Dtest=PinotTableRestletResourceTest#testTableTasksCleanupWithNonActiveTasks -Dsurefire.failIfNoSpecifiedTests=false test -DskipITs -DskipIntegrationTests./mvnw -pl pinot-controller -Dtest=PinotTableRestletResourceTest#testTableTasksCleanupWithActiveTasks -Dsurefire.failIfNoSpecifiedTests=false test -DskipITs -DskipIntegrationTests./mvnw -pl pinot-controller -Dtest=PinotTableRestletResourceTest -Dsurefire.failIfNoSpecifiedTests=false test -DskipITs -DskipIntegrationTests./mvnw -pl pinot-core -Dtest=TruncateDecimalTransformFunctionTest -Dsurefire.failIfNoSpecifiedTests=false test -DskipITs -DskipIntegrationTestsTruncateDecimalTransformFunctionTest#testTruncateDecimalTransformFunction(5 consecutive successful iterations)