Another attempt for the cache fix#6678
Conversation
* Improve cache push * Format
Codecov Report
@@ Coverage Diff @@
## master #6678 +/- ##
=======================================
+ Coverage 70% 70% +1%
=======================================
Files 351 351
Lines 30214 30107 -107
=======================================
- Hits 21132 21070 -62
+ Misses 8231 8195 -36
+ Partials 851 842 -9
Continue to review full report at Codecov.
|
ZackButcher
left a comment
There was a problem hiding this comment.
LG, just a nit or two.
| clearCacheMutex sync.Mutex | ||
| clearCacheTime = 1 | ||
|
|
||
| clearCacheMutex sync.Mutex |
There was a problem hiding this comment.
Put the mutex at the top of this var block since it protects all of the variables here (mirroring the pattern that a mutex struct field should come before the other struct fields it guards).
| }) | ||
| } | ||
| // If last config change was > 1 second ago, push. | ||
| if time.Since(lastClearCacheEvent) > 1*time.Second { |
There was a problem hiding this comment.
How/why did we pick 1 second as the flush interval? Second, please make this a variable called something like maxCacheClearInterval and store it above with the cache vars.
There was a problem hiding this comment.
Good point - I'll do both on follow-up PRs (since the bot and CI are taking many hours to run, and we're trying to get this into the monthly ) - I don't think either change are critical (or worse than the bug )
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: costinm, ZackButcher The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@costinm: The following test failed, say
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/test-infra repository. I understand the commands that are listed here. |
* Improve cache push * Format * Improve push squashing (istio#6641) * Improve cache push * Format * Revert experiment to verify if validation can be disabled
Fixes #5588 - the root problem was that events were squashed, but nothing triggered a push for the last events. So change #1 was pushed immediately, change #2 triggered a callback after (squash time).
Instead the callback is now after 1 second, and we evaluate if in the last second we received any event.
If not - push. Otherwise, keep waiting until the batch of changes settles down.