Enable look-up by secondary index in cache #4125

derekwaynecarr · 2015-02-04T20:52:12Z

This is an enhancement to our cache to let you optionally create a store that can build a secondary index on its data. The main use case is I want to have a store that holds a bunch of resources across namespaces, and I want to be able to then interface with that store by asking to retrieve only the resources in namespace X.

If folks are good with this pattern, I will adopt it in the admission control handlers for resource quota and limit range.

@smarterclayton @ironcladlou @erictune

ironcladlou · 2015-02-04T21:05:44Z

pkg/client/cache/store.go

+// it is intended to be called from a function that already has a lock on the cache
+func (c *cache) updateIndex(obj interface{}) error {
+	if c.indexFunc == nil {
+		return nil


Since Cache claims to implements Index, I feel like a nil indexFunc would be an illegal state resulting in silently broken indexing behavior. Maybe this should return an error?

I could see that. I guess I could have a default indexFunc that maps to the empty string.

ironcladlou · 2015-02-04T21:11:40Z

Really cool idea. I made some comments inline.

derekwaynecarr · 2015-02-05T16:16:36Z

@ironcladlou thanks for the feedback, will update per your comments.

after speaking with @markturansky , we thought it would be useful to support multiple index funcs. I am going to take another pass at this work with that concept in mind.

markturansky · 2015-02-05T16:19:28Z

The single index function could also be a composite of index functions.

derekwaynecarr · 2015-02-05T18:26:52Z

Added support for giving multiple index functions, and then ability to get an index by its name.

Addressed @ironcladlou feedback to make sure cache works fine as both an Indexer and a Store.

Please review and provide any further feedback.

deads2k · 2015-02-06T16:07:53Z

pkg/client/cache/store.go

@@ -76,6 +81,57 @@ func (c *cache) Add(obj interface{}) error {
 	c.lock.Lock()
 	defer c.lock.Unlock()
 	c.items[key] = obj
+	c.updateIndices(obj)


Can you build the list of what you need to modify in the indexes separately from actually making the update? It would help limit contention on the lock. Same reason the key is calculated before lock acquisition.

I don't think rebuilding the entire index is going to scale very long. Why not start with incremental? Get prev object, gen index entries for it, delete those, gen index entries for new object, insert those.

On Feb 6, 2015, at 11:08 AM, David Eads notifications@github.com wrote:

In pkg/client/cache/store.go:

@@ -76,6 +81,57 @@ func (c *cache) Add(obj interface{}) error {
c.lock.Lock()
defer c.lock.Unlock()
c.items[key] = obj

c.updateIndices(obj)
Can you build the list of what you need to modify in the indexes separately from actually making the update? It would help limit contention on the lock. Same reason the key is calculated before lock acquisition.

—
Reply to this email directly or view it on GitHub.

@smarterclayton - its not rebuilding the entire index. your point on prev object is needed, will update.

basically, i need a diff call on add vs update.

so if on update, i get the previous object based on its key, and then remove that object from all indices, then for updated object treat like an add, it should be fine. will make an update this afternoon.

Right.

----- Original Message -----

@@ -76,6 +81,57 @@ func (c *cache) Add(obj interface{}) error {
c.lock.Lock()
defer c.lock.Unlock()
c.items[key] = obj

c.updateIndices(obj)

so if on update, i get the previous object based on its key, and then remove
that object from all indices, then for updated object treat like an add, it
should be fine. will make an update this afternoon.

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/4125/files#r24257444

You might have to hold a lock while doing that (or the Add has to fail fast if there's a conflict and you have to try again)

----- Original Message -----

@@ -76,6 +81,57 @@ func (c *cache) Add(obj interface{}) error {
c.lock.Lock()
defer c.lock.Unlock()
c.items[key] = obj

c.updateIndices(obj)

so if on update, i get the previous object based on its key, and then remove
that object from all indices, then for updated object treat like an add, it
should be fine. will make an update this afternoon.

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/4125/files#r24257444

derekwaynecarr · 2015-02-09T21:40:44Z

Updated to ensure that when updating an object in the indices that it handles removal before addition. Current indexing is incremental. An obvious solution to limiting lock contention is not there for me right now because I need the oldObject in the context of a lock in order to do removal. I think this is a good first step and should not block the PR.

derekwaynecarr · 2015-02-12T00:22:52Z

Any other comments or issues with this PR?

smarterclayton · 2015-02-12T00:58:21Z

LGTM

erictune · 2015-02-12T06:26:23Z

LGTM

erictune · 2015-02-12T06:27:13Z

will merge this tomorrow if the oncall doesn't get to it first.

Enable look-up by secondary index in cache

ironcladlou reviewed Feb 4, 2015
View reviewed changes

cjcullen assigned dchen1107 Feb 4, 2015

derekwaynecarr force-pushed the store_by_namespace branch 2 times, most recently from 2e6f9c9 to 09cb617 Compare February 5, 2015 18:25

derekwaynecarr force-pushed the store_by_namespace branch from 09cb617 to b0d388e Compare February 5, 2015 18:32

deads2k reviewed Feb 6, 2015
View reviewed changes

derekwaynecarr force-pushed the store_by_namespace branch from b0d388e to ec5e794 Compare February 9, 2015 21:38

derekwaynecarr added 3 commits February 9, 2015 16:44

Enable look-up by secondary index in cache

7a2d630

Support multiple index functions, address feedback

1cf69bd

Make sure we remove old object from index before adding new

53f1efa

derekwaynecarr force-pushed the store_by_namespace branch from ec5e794 to 53f1efa Compare February 9, 2015 21:45

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2015

erictune assigned erictune and unassigned dchen1107 Feb 12, 2015

smarterclayton added a commit that referenced this pull request Feb 12, 2015

Merge pull request #4125 from derekwaynecarr/store_by_namespace

371349f

Enable look-up by secondary index in cache

smarterclayton merged commit 371349f into kubernetes:master Feb 12, 2015

derekwaynecarr deleted the store_by_namespace branch April 17, 2015 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable look-up by secondary index in cache #4125

Enable look-up by secondary index in cache #4125

derekwaynecarr commented Feb 4, 2015

ironcladlou Feb 4, 2015

derekwaynecarr Feb 4, 2015

ironcladlou commented Feb 4, 2015

derekwaynecarr commented Feb 5, 2015

markturansky commented Feb 5, 2015

derekwaynecarr commented Feb 5, 2015

deads2k Feb 6, 2015

smarterclayton Feb 6, 2015

derekwaynecarr Feb 6, 2015

derekwaynecarr Feb 6, 2015

derekwaynecarr Feb 6, 2015

smarterclayton Feb 6, 2015

smarterclayton Feb 6, 2015

derekwaynecarr commented Feb 9, 2015

derekwaynecarr commented Feb 12, 2015

smarterclayton commented Feb 12, 2015

erictune commented Feb 12, 2015

erictune commented Feb 12, 2015

Enable look-up by secondary index in cache #4125

Enable look-up by secondary index in cache #4125

Conversation

derekwaynecarr commented Feb 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironcladlou commented Feb 4, 2015

derekwaynecarr commented Feb 5, 2015

markturansky commented Feb 5, 2015

derekwaynecarr commented Feb 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Feb 9, 2015

derekwaynecarr commented Feb 12, 2015

smarterclayton commented Feb 12, 2015

erictune commented Feb 12, 2015

erictune commented Feb 12, 2015