Use cached discovery interface #1422
Conversation
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
ANeumann82
requested review from
alenkacz,
gerred,
jbarrick-mesosphere,
kensipe,
nfnt and
zen-dog
as code owners
pkg/engine/resource/object_key.go
Outdated
| // Fetch namespaced API resources | ||
| _, apiResources, err := di.ServerGroupsAndResources() | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to fetch server groups and resources: %v", err) | ||
| if err == memory.ErrCacheNotFound { | ||
| di.Invalidate() |
There was a problem hiding this comment.
Please add a logging line here so we could track cache misses.
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
…esources from DiscoveryInterface Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
ANeumann82
force-pushed
the
an/use-cached-discovery
branch
from
fa52f2b
to
8ab533f
Compare
| @@ -58,7 +58,7 @@ import ( | |||
| // Reconciler reconciles an Instance object. | |||
| type Reconciler struct { | |||
| client.Client | |||
| Discovery discovery.DiscoveryInterface | |||
| Discovery discovery.CachedDiscoveryInterface | |||
There was a problem hiding this comment.
we can use the same discovery.DiscoveryInterface and change the impl
There was a problem hiding this comment.
offline conversation summary:
It seems best to not force cache retry checks on all users of discovery. We should provide our own impl that implements DiscoveryInterface and delegates to memory cache and handles the logic we are looking for... assuming that is the logic we consistently desired.
|
@ANeumann82 I'm not sure where this PR stands... it looks like #1423 might have done what we were looking for. If not.. this needs to be rebased. Can we close this? |
#1423 alleviate the problem but we'll still need caching there. I'd rather we come to an agreement or agree-to-disagree and use |
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> # Conflicts: # pkg/engine/resource/object_key.go # pkg/test/step_test.go
|
@kensipe I've merged master, so this should be up-to-date. I agree with @zen-dog that we probably need caching, and I do agree that we shouldn't hide Caching in a DiscoveryInterface - having the Another option would be to have a custom caching for the isNamespaced layer, but I don't think that would make it easier to understand or to develop. I've created a ticket #1433 where we should look into metrics to keep these things checked in future testing. |
Signed-off-by: Andreas Neumann aneumann@mesosphere.com
What this PR does / why we need it:
Using the non-cached version introduces a ~1 second delay on every call to
isNamespaced, includingObjectKeyFromObject.Fixes #1421