etcd watch cache for origin types #9057
Conversation
|
[test] |
liggitt
changed the title
|
@deads2k PTAL |
|
[test] |
|
oh good, a small one :) |
| store := &etcdgeneric.Etcd{ | ||
| NewFunc: func() runtime.Object { return &authorizationapi.ClusterPolicy{} }, | ||
| NewListFunc: func() runtime.Object { return &authorizationapi.ClusterPolicyList{} }, | ||
| QualifiedResource: authorizationapi.Resource("clusterpolicy"), | ||
| QualifiedResource: authorizationapi.Resource("clusterpolicies"), |
There was a problem hiding this comment.
Most of the time singular is correct: create, delete, update, get (and their multiple flavors) all singular. Only list is plural. I'd prefer the common case look right.
There was a problem hiding this comment.
Most of the time singular is correct:
Joy, inconsistent in our code.
There was a problem hiding this comment.
if we're using this to look up configured storage, which would you expect?
|
Looks like the upstream API could use some significant work. It seems like this object can't decide what its supposed to be doing or how its supposed to be getting built. I'm fine with the way it's wired in. I'd be surprised if end up suggesting people change these. |
| @@ -21,11 +22,12 @@ type REST struct { | |||
| } | |||
|
|
|||
| // NewStorage returns a RESTStorage object that will work against nodes. | |||
| func NewStorage(s storage.Interface) *REST { | |||
| func NewStorage(optsGetter restoptions.Getter) *REST { | |||
There was a problem hiding this comment.
the rebase is changing this for every NewStorage to use generic.RESTOption to sync it with upstream, I wonder how the restoptions.Getter is different.
There was a problem hiding this comment.
we want to look up options for a specific resource, so we can do things like shard etcd, have different watch cache enablement or size settings, etc.
There was a problem hiding this comment.
@liggitt go, so generally it won't be difficult to adapt the Getter to work with generic.RESTOption and we can do it on one place (I think the Getter will have to implement the RESTOption interface).
There was a problem hiding this comment.
RESTOption isn't an interface. This Getter allows the storage to obtain a RESTOption struct, and we'll apply it to the generic etcd struct in one place
|
clean test run other than TestImageStreamImport issue https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4296/ |
|
@deads2k comments addressed, other than plural QualifiedResource |
|
clean test run other than TestImageStreamImport issue https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4300/ |
|
lgtm |
|
lgtm as well (will fix the rebase), @liggitt I guess this is needed for https://bugzilla.redhat.com/show_bug.cgi?id=1333932 right? |
|
@mfojtik yes, only reason I'm merging it now. [test][merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4313/) (Image: devenv-rhel7_4301) |
|
Evaluated for origin merge up to 353cce3 |
|
Evaluated for origin test up to 353cce3 |
|
@liggitt I think the merge is still b0rken, @smarterclayton are you going to unblock us by disabling the flaking integration tests? |
|
He force merged the integration fix already (#9118) |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4313/) |
No description provided.