-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
statistics: avoid frequantly syncing stats simultaneously #54480
base: master
Are you sure you want to change the base?
statistics: avoid frequantly syncing stats simultaneously #54480
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #54480 +/- ##
=================================================
- Coverage 72.8089% 54.5071% -18.3019%
=================================================
Files 1549 1664 +115
Lines 436313 604108 +167795
=================================================
+ Hits 317675 329282 +11607
- Misses 99059 252987 +153928
- Partials 19579 21839 +2260
Flags with carried forward coverage won't be shown. Click here to find out more.
|
We had some offline discussions that this change could potentially cause a table to be re-analyzed after the analysis is executed. The update method is called after the analysis, but if we only do it once, there is a big chance that the stats cache of the analyzed table will not be updated, which will result in the table being re-queued for analysis. |
46ab584
to
e74d6ab
Compare
func (*StatsCacheImpl) Update() error { | ||
select { | ||
case StatsCacheUpdateChan <- struct{}{}: | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it doesn't solve the issue I mentioned in #54480 (comment).
I believe we need to ensure that once the analysis is finished, we can always load the latest stats for the analyzed tables. Otherwise, there is a chance we will reanalyze them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you. Therefore, prior to commencing auto analyze, I will enforce a synchronization of the stats cache.
[LGTM Timeline notifier]Timeline:
|
19c50c8
to
f7ff972
Compare
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
f7ff972
to
cf10f85
Compare
cf10f85
to
cfca74a
Compare
Signed-off-by: Weizhen Wang <[email protected]>
Signed-off-by: Weizhen Wang <[email protected]>
Signed-off-by: Weizhen Wang <[email protected]>
Signed-off-by: Weizhen Wang <[email protected]>
Signed-off-by: Weizhen Wang <[email protected]>
Signed-off-by: Weizhen Wang <[email protected]>
358504f
to
9d0c900
Compare
Signed-off-by: Weizhen Wang <[email protected]>
Signed-off-by: Weizhen Wang <[email protected]>
Signed-off-by: Weizhen Wang <[email protected]>
@hawkingrei: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
@hawkingrei: The following tests failed, say
Full PR test history. Your PR dashboard. 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/test-infra repository. I understand the commands that are listed here. |
PR needs rebase. 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/test-infra repository. |
What problem does this PR solve?
Issue Number: close #54481
Problem Summary:
What changed and how does it work?
Now,
StatsCacheImpl.Update
can sync stats by version from the storage. we have many scenarios to call it. such asdomain.loadStatsWorker
andAnalyze
. When customers use lightning, lightning will trigger the multi-analyzed tasks simultaneously. so it also triggers multiStatsCacheImpl.Update
simultaneously. it is unnecessary.add a singleflight for
StatsCacheImpl.Update
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.