-
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.8103% 56.0265% -16.7839%
=================================================
Files 1553 1674 +121
Lines 437420 609590 +172170
=================================================
+ Hits 318487 341532 +23045
- Misses 99331 244724 +145393
- Partials 19602 23334 +3732
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
[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
358504f
to
9d0c900
Compare
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. |
1b7ed28
to
1d80328
Compare
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
0317fc2
to
e99e4c3
Compare
@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.