11945: Key collection cache on pdh+token.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curoverse.com>

diff --git a/services/keep-web/cache.go b/services/keep-web/cache.go
index d72effc075d65a62d41b722ec177dfa3d465ec5f..c29ae7d43c270b129c0bf459aab25df927dc0407 100644 (file)
--- a/services/keep-web/cache.go
+++ b/services/keep-web/cache.go
@@ -118,7 +118,7 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
 
        var collection *arvados.Collection
        if pdh != "" {
-               collection = c.lookupCollection(pdh)
+               collection = c.lookupCollection(arv.ApiToken + "\000" + pdh)
        }
 
        if collection != nil && permOK {
@@ -150,7 +150,7 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
                        // PDH changed, but now we know we have
                        // permission -- and maybe we already have the
                        // new PDH in the cache.
-                       if coll := c.lookupCollection(current.PortableDataHash); coll != nil {
+                       if coll := c.lookupCollection(arv.ApiToken + "\000" + current.PortableDataHash); coll != nil {
                                return coll, nil
                        }
                }
@@ -170,14 +170,13 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
                expire: exp,
                pdh:    collection.PortableDataHash,
        })
-       // Disabled, see #11945
-       // c.collections.Add(collection.PortableDataHash, &cachedCollection{
-       //      expire:     exp,
-       //      collection: collection,
-       // })
-       // if int64(len(collection.ManifestText)) > c.MaxCollectionBytes/int64(c.MaxCollectionEntries) {
-       //      go c.pruneCollections()
-       // }
+       c.collections.Add(arv.ApiToken+"\000"+collection.PortableDataHash, &cachedCollection{
+               expire:     exp,
+               collection: collection,
+       })
+       if int64(len(collection.ManifestText)) > c.MaxCollectionBytes/int64(c.MaxCollectionEntries) {
+               go c.pruneCollections()
+       }
        return collection, nil
 }
 
@@ -238,15 +237,13 @@ func (c *cache) collectionBytes() uint64 {
        return size
 }
 
-func (c *cache) lookupCollection(pdh string) *arvados.Collection {
-       if pdh == "" {
-               return nil
-       } else if ent, cached := c.collections.Get(pdh); !cached {
+func (c *cache) lookupCollection(key string) *arvados.Collection {
+       if ent, cached := c.collections.Get(key); !cached {
                return nil
        } else {
                ent := ent.(*cachedCollection)
                if ent.expire.Before(time.Now()) {
-                       c.collections.Remove(pdh)
+                       c.collections.Remove(key)
                        return nil
                } else {
                        atomic.AddUint64(&c.stats.CollectionHits, 1)

diff --git a/services/keep-web/cache_test.go b/services/keep-web/cache_test.go
index 05325270e6bd3e566037c0a899ceeac5d7600275..cddeaf489763500b9e7230a75c2b19a4c25f40cf 100644 (file)
--- a/services/keep-web/cache_test.go
+++ b/services/keep-web/cache_test.go
@@ -5,14 +5,13 @@
 package main
 
 import (
+       "git.curoverse.com/arvados.git/sdk/go/arvados"
        "git.curoverse.com/arvados.git/sdk/go/arvadosclient"
        "git.curoverse.com/arvados.git/sdk/go/arvadostest"
        "gopkg.in/check.v1"
 )
 
 func (s *UnitSuite) TestCache(c *check.C) {
-       c.Skip("see #11945")
-
        arv, err := arvadosclient.MakeArvadosClient()
        c.Assert(err, check.Equals, nil)
 
@@ -22,8 +21,9 @@ func (s *UnitSuite) TestCache(c *check.C) {
        // the first req should cause an API call; the next 4 should
        // hit all caches.
        arv.ApiToken = arvadostest.AdminToken
+       var coll *arvados.Collection
        for i := 0; i < 5; i++ {
-               coll, err := cache.Get(arv, arvadostest.FooCollection, false)
+               coll, err = cache.Get(arv, arvadostest.FooCollection, false)
                c.Check(err, check.Equals, nil)
                c.Assert(coll, check.NotNil)
                c.Check(coll.PortableDataHash, check.Equals, arvadostest.FooPdh)
@@ -37,18 +37,32 @@ func (s *UnitSuite) TestCache(c *check.C) {
 
        // Hit the same collection 2 more times, this time requesting
        // it by PDH and using a different token. The first req should
-       // miss the permission cache. Both reqs should hit the
-       // Collection cache and skip the API lookup.
+       // miss the permission cache and fetch the new manifest; the
+       // second should hit the Collection cache and skip the API
+       // lookup.
        arv.ApiToken = arvadostest.ActiveToken
-       for i := 0; i < 2; i++ {
-               coll, err := cache.Get(arv, arvadostest.FooPdh, false)
-               c.Check(err, check.Equals, nil)
-               c.Assert(coll, check.NotNil)
-               c.Check(coll.PortableDataHash, check.Equals, arvadostest.FooPdh)
-               c.Check(coll.ManifestText[:2], check.Equals, ". ")
-       }
+
+       coll2, err := cache.Get(arv, arvadostest.FooPdh, false)
+       c.Check(err, check.Equals, nil)
+       c.Assert(coll2, check.NotNil)
+       c.Check(coll2.PortableDataHash, check.Equals, arvadostest.FooPdh)
+       c.Check(coll2.ManifestText[:2], check.Equals, ". ")
+       c.Check(coll2.ManifestText, check.Not(check.Equals), coll.ManifestText)
+
+       c.Check(cache.Stats().Requests, check.Equals, uint64(5+1))
+       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4+0))
+       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(4+0))
+       c.Check(cache.Stats().PDHHits, check.Equals, uint64(4+0))
+       c.Check(cache.Stats().APICalls, check.Equals, uint64(1+1))
+
+       coll2, err = cache.Get(arv, arvadostest.FooPdh, false)
+       c.Check(err, check.Equals, nil)
+       c.Assert(coll2, check.NotNil)
+       c.Check(coll2.PortableDataHash, check.Equals, arvadostest.FooPdh)
+       c.Check(coll2.ManifestText[:2], check.Equals, ". ")
+
        c.Check(cache.Stats().Requests, check.Equals, uint64(5+2))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4+2))
+       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4+1))
        c.Check(cache.Stats().PermissionHits, check.Equals, uint64(4+1))
        c.Check(cache.Stats().PDHHits, check.Equals, uint64(4+0))
        c.Check(cache.Stats().APICalls, check.Equals, uint64(1+1))
@@ -67,15 +81,13 @@ func (s *UnitSuite) TestCache(c *check.C) {
                c.Check(err, check.Equals, nil)
        }
        c.Check(cache.Stats().Requests, check.Equals, uint64(5+2+20))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4+2+18))
+       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4+1+18))
        c.Check(cache.Stats().PermissionHits, check.Equals, uint64(4+1+18))
        c.Check(cache.Stats().PDHHits, check.Equals, uint64(4+0+18))
        c.Check(cache.Stats().APICalls, check.Equals, uint64(1+1+2))
 }
 
 func (s *UnitSuite) TestCacheForceReloadByPDH(c *check.C) {
-       c.Skip("see #11945")
-
        arv, err := arvadosclient.MakeArvadosClient()
        c.Assert(err, check.Equals, nil)
 
@@ -94,8 +106,6 @@ func (s *UnitSuite) TestCacheForceReloadByPDH(c *check.C) {
 }
 
 func (s *UnitSuite) TestCacheForceReloadByUUID(c *check.C) {
-       c.Skip("see #11945")
-
        arv, err := arvadosclient.MakeArvadosClient()
        c.Assert(err, check.Equals, nil)