Squash clear cache calls #3506
Squash clear cache calls #3506
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
| @@ -481,6 +489,19 @@ func (ds *DiscoveryService) ClearCacheStats(_ *restful.Request, _ *restful.Respo | |||
| // clearCache will clear all envoy caches. Called by service, instance and config handlers. | |||
| // This will impact the performance, since envoy will need to recalculate. | |||
| func (ds *DiscoveryService) clearCache() { | |||
| clearCacheMutex.Lock() | |||
There was a problem hiding this comment.
no need for mutex, this is run on a single event queue
There was a problem hiding this comment.
Just feel safer with a mutex - with the other changes in multicluster and other optimizations
we may have multiple callers.
There was a problem hiding this comment.
how does this mutex work with the cache lock? there are things reading from the cache in parallel, right?
There was a problem hiding this comment.
This mutex only syncs the clearCache calls.
Additional mutexes or other sync may be needed - we'll discuss it when that work is done, for now
it's useful to stop assuming everything is in one thread... Is there any harm in adding a mutex to clearCache ?
| if time.Since(lastClearCache) < 60*time.Second { | ||
| if !clearCacheTimerSet { | ||
| clearCacheTimerSet = true | ||
| time.AfterFunc(61*time.Second, func() { |
There was a problem hiding this comment.
isn't this going to block the event queue?
There was a problem hiding this comment.
the danger here is the unbound growth of the incoming events. i think we need to run a secondary worker thread to refresh the cache like @rshriram suggested. they only need to share one bit for the "dirtiness" of the cache.
|
@costinm PR needs rebase |
istio.VERSION
Outdated
| export MIXER_TAG="replace-by-actual-tag" | ||
| export PILOT_HUB="gcr.io/istio-testing" | ||
| export PILOT_TAG="replace-by-actual-tag" | ||
| export CA_HUB="costinm" |
There was a problem hiding this comment.
please look at your PR/diff before and after submission :-)
There was a problem hiding this comment.
Ah, istio.VERSION again...
|
No, extra mutexes are only dangerous if they introduce dead locks :)
…On Wed, Feb 14, 2018 at 5:48 PM Costin Manolache ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pilot/pkg/proxy/envoy/v1/discovery.go
<#3506 (comment)>:
> @@ -481,6 +489,19 @@ func (ds *DiscoveryService) ClearCacheStats(_ *restful.Request, _ *restful.Respo
// clearCache will clear all envoy caches. Called by service, instance and config handlers.
// This will impact the performance, since envoy will need to recalculate.
func (ds *DiscoveryService) clearCache() {
+ clearCacheMutex.Lock()
This mutex only syncs the clearCache calls.
Additional mutexes or other sync may be needed - we'll discuss it when
that work is done, for now
it's useful to stop assuming everything is in one thread... Is there any
harm in adding a mutex to clearCache ?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#3506 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJGIxsXuGj4ssJwC24LO5PLaaCDeNvmXks5tU40HgaJpZM4SGL46>
.
|
| clearCacheMutex.Lock() | ||
| defer clearCacheMutex.Unlock() | ||
|
|
||
| if time.Since(lastClearCache) < time.Duration(clearCacheTime) * time.Second { |
There was a problem hiding this comment.
If the time since last clear is less than x seconds - we schedule the event.
I think it's <
|
@costinm: The following test failed, say
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. |
|
ping ? |
|
I would rename this clear cache squash to batchCacheEvictionEvents for clarity. |
|
Agreed, will try to do it in the next PR related to this ( to avoid another lgtm/approve/build cycle...) |
|
the errors in https://k8s-gubernator.appspot.com/build/istio-prow/pull/istio_istio/3506/e2e-smoke/3037/ seems a bit concerning |
Based on test results, in separate PR I may make it customisable, for now looking to see if it has
any impact on CPU/memory use.