Refactor cache package. #29

pwittrock · 2018-06-13T04:48:40Z

Follow up with tests for the cache and cache/internal packages.

pwittrock · 2018-06-13T15:17:32Z

@seans3 This is the other half of the client PR. Once the structure looks good, we should add thorough testing.

DirectXMan12

some comments inline. General approach is ok. Creating informers after start is questionable. Mutex use needs review.

DirectXMan12 · 2018-06-13T18:39:28Z

pkg/cache/cache.go

-	Indexer cache.Indexer
-	// GroupVersionKind is the group-version-kind of the resource.
-	GroupVersionKind schema.GroupVersionKind
+	// IndexField adds an index to a field.


This probably needs to copy the rest of the godoc from the related fields about the what you should pass to extractValue and the caveats, or at least have a note about it.

Added a go doc from client-go. LMK if this is the right one.

DirectXMan12 · 2018-06-13T18:41:40Z

pkg/cache/cache.go

+// New initializes and returns a new Cache
+func New(config *rest.Config, opts Options) (Cache, error) {
+	// Use the default Kubernetes Scheme if unset
+	if opts.Scheme == nil {


refactor this out into an Options.ApplyDefaults method.

DirectXMan12 · 2018-06-13T18:47:17Z

pkg/cache/internal/factory.go

+		ip.informersByGVK[gvk] = i
+
+		// Start the Informer if need by
+		if ip.started {


hmm... I'd be a bit wary of this. Certain expectations break if this happens (e.g. you can't add indexers, and I'd need to double check what happens if you add new event handlers -- do you get the initial populate events)

Added a todo about adding tests and documentation for this.

DirectXMan12 · 2018-06-13T19:05:43Z

pkg/cache/internal/factory.go

+	// to be locked
+	var sync bool
+	i, err := func() (*MapEntry, error) {
+		ip.mu.Lock()


This is going to cause issues, because it means that only one read operation can happen across any index at once, which is suboptimal. That's one of the reasons why the separation between creation and client lookup was useful. I think what we might want to do is

// assume the happy path first with read-only-lock { if i, ok := informers[gvk]; ok { return i } } // sad path, first time only with write-lock { // check again in case we raced if i, ok := informers[gvk]; ok { return i } // only construct if we actually need to do so i := construct informer informers[gvk] = i return i }

pwittrock · 2018-06-13T20:30:40Z

PTAL

seans3 · 2018-06-13T22:53:56Z

pkg/cache/cache.go

-	Indexer cache.Indexer
-	// GroupVersionKind is the group-version-kind of the resource.
-	GroupVersionKind schema.GroupVersionKind
+// cache is a Kubernetes Object cache populated from InformersMap.  cache wraps a CacheProvider and InformersMap.


cache -> informerCache

seans3 · 2018-06-13T22:54:43Z

pkg/cache/cache.go

-	Indexer cache.Indexer
-	// GroupVersionKind is the group-version-kind of the resource.
-	GroupVersionKind schema.GroupVersionKind
+// cache is a Kubernetes Object cache populated from InformersMap.  cache wraps a CacheProvider and InformersMap.


It looks like informerCache only wraps an InformersMap, not a CacheProvider as stated in comment.

seans3 · 2018-06-13T22:57:01Z

pkg/cache/cache.go

-		return k.Name
+	// Default the resync period to 10 hours if unset
+	if opts.Resync == nil {
+		r := 10 * time.Hour


Consider pulling 10 out into a const as "defaultResyncHours"

droot

Looks good to me.

seans3 · 2018-06-13T23:06:05Z

pkg/cache/internal/factory.go

+)
+
+// NewInformersMap returns a new InformersMap
+func NewInformersMap(config *rest.Config,


If this file is mostly about the InformersMap, why is it called factory.go ?

seans3 · 2018-06-13T23:10:24Z

pkg/cache/internal/factory.go

+	return cache.WaitForCacheSync(stop, syncedFuncs...)
+}
+
+// Get will create a new Informer and added it to the map of InformersMap if none exists.  Returns


nit: added -> add

seans3 · 2018-06-13T23:11:51Z

pkg/cache/internal/factory.go

+	}
+
+	// Do the mutex part in its own function so we can use defer without blocking pieces that don't
+	// to be locked


nit: missing "need" to be locked.

seans3 · 2018-06-13T23:16:04Z

pkg/cache/internal/typed_reader.go

+var _ client.Reader = &Reader{}
+
+// Reader wraps a cache.Index to implement the client.Reader interface for a single type
+type Reader struct {


Reader struct might be confused with Reader interface. You might want to consider another name.

seans3 · 2018-06-13T23:20:37Z

pkg/cache/reader.go

+var _ client.Reader = &informerCache{}
+
+// Get implements Reader
+func (ip *informerCache) Get(ctx context.Context, key client.ObjectKey, out runtime.Object) error {


Even though the informerCache implements the client.Reader interface, would this file more accurately be called informer_cache.go instead of reader.go ?

seans3

Mostly nits. Not sure why tests in cache_test are commented out instead of removed.

seans3 · 2018-06-13T23:51:20Z

/lgtm
/approve

pwittrock

Tests are commented out because we should uncomment them and make them pass, but they weren't running. Fixing the tests should be the next priority.

pwittrock · 2018-06-14T03:21:58Z

Comments addressed in #35

Fixup pkg/cache with comments from #29

Refactor cache package.

Fixup pkg/cache with comments from kubernetes-sigs#29

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 13, 2018

pwittrock requested a review from seans3 June 13, 2018 04:48

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 13, 2018

pwittrock force-pushed the cache branch 2 times, most recently from 3f7d90d to bf3559e Compare June 13, 2018 14:38

pwittrock assigned seans3 and DirectXMan12 Jun 13, 2018

pwittrock force-pushed the cache branch from bf3559e to 827c00f Compare June 13, 2018 14:49

DirectXMan12 suggested changes Jun 13, 2018

View reviewed changes

pwittrock added 3 commits June 13, 2018 13:23

Refactor cache package.

b0a9eb8

Merge cache and informers into a single type.

0b7f6ac

Address comments

8cf25e7

pwittrock force-pushed the cache branch from 827c00f to 2563bf9 Compare June 13, 2018 20:26

fix rebase issues

51ed3cb

pwittrock force-pushed the cache branch from 2563bf9 to 51ed3cb Compare June 13, 2018 20:43

seans3 reviewed Jun 13, 2018

View reviewed changes

droot reviewed Jun 13, 2018

View reviewed changes

seans3 reviewed Jun 13, 2018

View reviewed changes

seans3 suggested changes Jun 13, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2018

pwittrock added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2018

k8s-ci-robot merged commit bdb5f7c into kubernetes-sigs:master Jun 14, 2018

pwittrock commented Jun 14, 2018

View reviewed changes

pwittrock added a commit to pwittrock/controller-runtime that referenced this pull request Jun 14, 2018

Fixup pkg/cache with comments from kubernetes-sigs#29

ebb0e8a

k8s-ci-robot added a commit that referenced this pull request Jun 14, 2018

Merge pull request #35 from pwittrock/cache

0216abd

Fixup pkg/cache with comments from #29

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#29 from pwittrock/cache

dbdeb47

Refactor cache package.

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Fixup pkg/cache with comments from kubernetes-sigs#29

916b3aa

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#35 from pwittrock/cache

aa18517

Fixup pkg/cache with comments from kubernetes-sigs#29

Refactor cache package. #29

Refactor cache package. #29

Uh oh!

Conversation

pwittrock commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pwittrock commented Jun 13, 2018

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pwittrock commented Jun 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seans3 Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seans3 left a comment

Choose a reason for hiding this comment

Uh oh!

seans3 commented Jun 13, 2018

Uh oh!

pwittrock left a comment

Choose a reason for hiding this comment

Uh oh!

pwittrock commented Jun 14, 2018

Uh oh!

Uh oh!

pwittrock commented Jun 13, 2018 •

edited

Loading

seans3 Jun 13, 2018 •

edited

Loading