Repository navigation
planner: flaky test in stats test - #65014
terry1purcell wants to merge 4 commits into
Conversation
|
Hi @terry1purcell. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a flaky test in the statistics test suite by addressing a race condition related to lazy loading of histogram metadata. The fix ensures that the ColAndIdxExistenceMap is fully populated before attempting to load histograms.
Key changes:
- Added verification steps to ensure table stats are properly cached after
h.Update()calls - Introduced a second
h.Update()call after priming queries to ensure the existence map is populated - Added assertions to verify that column metadata is correctly marked as analyzed in the existence map
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #65014 +/- ##
================================================
- Coverage 70.9290% 69.7071% -1.2220%
================================================
Files 1888 1889 +1
Lines 515954 525364 +9410
================================================
+ Hits 365961 366216 +255
- Misses 125582 135715 +10133
+ Partials 24411 23433 -978
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
@purelind: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/hold further investigation on root cause needed |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| // Also check the existence map as a fallback: if the existence map says the index is analyzed | ||
| // but the index object itself doesn't reflect that (e.g., due to lazy loading timing), we should | ||
| // still load it. This is particularly important for prefix indexes which might be created with | ||
| // lazy loading before the existence map is fully populated. | ||
| if t.ColAndIdxExistenceMap.HasAnalyzed(id, true) && !idx.IsFullLoad() { | ||
| return idx, true |
There was a problem hiding this comment.
The logic in lines 928-930 and 935-937 is redundant. If idx.IsAnalyzed() returns true (meaning idx.StatsVer != Version0), then t.ColAndIdxExistenceMap.HasAnalyzed(id, true) should also return true for a properly maintained existence map. The second check (lines 935-937) will only ever trigger when idx.IsAnalyzed() is false but HasAnalyzed is true, which suggests a data consistency issue between the index object's StatsVer and the existence map.
Consider simplifying this to a single condition or adding a data consistency check/warning if the two states are mismatched.
| // Also check the existence map as a fallback: if the existence map says the index is analyzed | |
| // but the index object itself doesn't reflect that (e.g., due to lazy loading timing), we should | |
| // still load it. This is particularly important for prefix indexes which might be created with | |
| // lazy loading before the existence map is fully populated. | |
| if t.ColAndIdxExistenceMap.HasAnalyzed(id, true) && !idx.IsFullLoad() { | |
| return idx, true | |
| // Data consistency check: if the existence map says analyzed but the index does not, log a warning. | |
| if t.ColAndIdxExistenceMap.HasAnalyzed(id, true) && !idx.IsAnalyzed() { | |
| fmt.Printf("[stats] Data inconsistency: index %d existence map says analyzed, but StatsVer is not. Table: %s\n", id, t.Info.Name.O) |
|
@terry1purcell: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What problem does this PR solve?
Issue Number: ref #63501
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.