HIVE-29642: Remove racy test-only counters from PartitionManagementTask#6520
Merged
Conversation
asf-ci-hive
added
tests passed
tests pending
and removed
tests pending
tests passed
labels
|
Contributor
Author
|
@saihemanth-cloudera, could you take a look when you have a moment, please? It's a small fix (HIVE-29642), CI is green — removes the racy test-only counters in PartitionManagementTask behind the intermittent TestPartitionManagement failures. Happy to address any feedback. pinging you because you've worked in this area recently (#5991) Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
HIVE-29642: Remove racy test-only counters from PartitionManagementTask
Remove the test-only static counters (completedAttempts, skippedAttempts) and their @VisibleForTesting accessors from PartitionManagementTask. Rewrite
TestPartitionManagement.testPartitionDiscoveryTransactionalTable (renamed to testPartitionDiscoveryTransactionalTableConcurrent to highlight the contention scenario) to verify the same properties
via a Log4j2 ListAppender that observes the existing skip and discovery log messages emitted by PartitionManagementTask.run().
Why are the changes needed?
The current test asserts that exactly 2 of 3 concurrent tasks are skipped, using test-only static counters in PartitionManagementTask. The assertion is flaky for two reasons. First, the JVM may
schedule the 3 tasks so they do not actually overlap, in which case no skips happen at all. Second, the counters themselves are racy: the skippedAttempts = 0 reset inside the lock can clear the
value while other threads are running skippedAttempts++ outside the lock. The counters were added in HIVE-20707 for this test and nothing in production reads them.
Does this PR introduce any user-facing change?
No. The only change in production source besides removing the counter fields is dropping the trailing "..#N" counter format from one INFO log message; the message prefix is unchanged.
How was this patch tested?
Ran TestPartitionManagement locally (11 tests, all pass). The counter race was empirically reproduced before the fix by inserting a short Thread.sleep right after lock.tryLock() in production,
which caused the original assertion to fail deterministically; the refactored test does not depend on this timing.