add support for caching (DCNE-680) by juchowan · Pull Request #148 · ciscoecosystem/mso-go-client

juchowan · 2025-12-17T16:55:49Z

No description provided.

akinross · 2025-12-18T11:09:20Z

client/client.go

 }

+// deepCloneContainer creates a true deep copy of a Container to prevent shared data races
+func (c *Client) deepCloneContainer(original *container.Container) (*container.Container, error) {


why expose this in client and not in gabs.go as a method/function on the container itself?

akinross · 2025-12-18T11:11:16Z

client/client.go

+}
+
+// GetSchemaWithCache retrieves schema with caching support
+func (c *Client) GetSchemaWithCache(schemaId string) (*container.Container, error) {


Should we make the cache mechanism generic to allow for anything to be cached with some sort of cache-id?

we could but im just not sure if it makes sense, i dont see any other object in ndo that could grow as much as schema to need caching, the new tenant templates have pretty limited configuration options so we wouldnt gain much from caching them

I would argue two point to this:

schema originally was not this big but it grew overtime, in some template structures like l3out templates there are already large configurations possible

from a consistency point of view I think the caching should behave similar for every GET request

akinross · 2025-12-18T11:15:09Z

client/client.go

 	return word
 }
+
+type ThreadSafeCache struct {


why add this too client file instead of as separate cache file?
do we have a non thread safe cache? should we just name it cache and allow have thread-safe implementation for the methods/functions?

akinross · 2025-12-18T11:16:48Z

client/client.go

+}
+
+// DeletePattern removes all items from the cache matching the pattern.
+func (c *ThreadSafeCache) DeletePattern(pattern string) {


do we also require a delete all or clear cache method/function?

there is InvalidateAllSchemaCache that can be used to clear all cache in case it's needed - will change that to Clear to have it for all possible cache. We also have InvalidateSchemaCache that is used after every create/update operation on schema resources
not sure there is need for anything more, closing the terraform sessions should clear cache on its own

akinross · 2025-12-18T11:18:43Z

client/client.go

+}
+
+// Set adds or updates an item in the cache.
+func (c *ThreadSafeCache) Set(key string, value interface{}) {


the c is kind of confusing me with the c used for client, shall we make this more specific?

akinross · 2025-12-18T11:29:41Z

client/client.go

+	items         map[string]interface{}
+	hits          int64
+	misses        int64
+	invalidations int64


What is your exact goal of this statistics part? should we also include statistics per cache item?
If we want to expose statistics I think we should create a statistics struct which is part of each item, with the necessary data.

type ThreadSafeCache struct { mu sync.RWMutex items map[string]CacheItem } type CacheItem struct { item interface{} hits int64 misses int64 invalidations int64 }

Perhaps even further where the statistics are also set in its own type.

Then have a cache statistics method/function that would provide a statistics report based on preference of statistics you would like to represent.

akinross · 2025-12-18T11:35:20Z

client/client.go

+	hits, misses, invalidations, hitRatio := c.Cache.GetStats()
+	log.Printf("[DEBUG] SCHEMA_CACHE_INVALIDATED for %s | Stats: Hits=%d, Misses=%d, Invalidations=%d, HitRatio=%.1f%%",
+		schemaId, hits, misses, invalidations, hitRatio)


why not leverage the LogCacheStats function, or some other wrapper function to prevent all the copied code?

akinross · 2025-12-18T11:40:18Z

README.md

+
+```golang
+// Fetch schema with caching support (automatically handles cache hits/misses)
+schema, err := msoClient.GetSchemaWithCache("schema-id")


why not adjust the current GetViaURL, so that when caching is enabled it leverage caching for everything?

for now i wanted to enabled it on selected resources only that we know are problematic to see the behaviour

I personally think we should have consistent behaviour on all the resources, but this is not a resource (schema) discussion in my opinion but a generic caching discussion. In this case I would say we should have something like a GetViaURLWithCache function that can be used for schema ( but also for other endpoints ) where it would access the caching mechanism.

In the provider we can then gradually change those resources.

akinross · 2025-12-18T11:40:59Z

README.md

+// Invalidate a specific schema from cache (e.g., after updates)
+msoClient.InvalidateSchemaCache("schema-id")
+
+// Clear all cached schemas (useful for bulk operations or cleanup)
+msoClient.InvalidateAllSchemaCache()
+
+// Get cache statistics for monitoring
+hits, misses, invalidations, hitRatio := msoClient.GetCacheStats()
+
+// Log current cache statistics
+msoClient.LogCacheStats()


I think these functions/methods should be part of a cache struct and not the client

akinross · 2026-02-23T09:30:43Z

client/cache.go

+const (
+	KB = 1024
+	MB = 1024 * 1024
+)


what are these constants? could you provide come comment/context for them?

akinross · 2026-02-23T09:34:32Z

client/cache.go

+	}
+}
+
+// Set adds or updates an item in the cache with per-item size tracking.


why are you introducing per item size tracking? is there a use case for this?

akinross · 2026-02-23T09:35:05Z

client/cache.go

+		itemSize = int64(len(jsonBytes))
+	} else {
+		// Fallback for non-byte values
+		itemSize = 1024 // Estimate 1KB for unknown types


why not use the defined constant for this?

akinross · 2026-02-23T09:40:37Z

client/client.go

+func CacheEnabled(enabled bool) Option {
+	return func(client *Client) {
+		client.cacheEnabled = enabled
+	}
+}
+


is this required? can't we init the client with a cache when we provide that option? this way when a cache exist on the client it is used and else it is not

akinross · 2026-02-23T09:43:50Z

client/client_service.go

+}
+
+// detectURLResourceType detects resource type from URL for better logging and monitoring
+func (c *Client) detectURLResourceType(url string) string {


what is the purpose of this? I see a lot of logging / statistics things being added when I think we should first align on the caching mechanism itself. Is the url itself not clear enough in logging?

akinross · 2026-02-23T09:50:57Z

client/cache.go

+}
+
+// Get gets and clones an item with per-item statistics tracking
+func (cache *Cache) Get(key string, cloneFunc func(interface{}) (interface{}, error)) (interface{}, bool, error) {


why do we need clonefunc? not sure i really see the benefit of passing in a function like this, why not handle this cloning if required inside of the get itself?

akinross · 2026-02-23T09:52:20Z

client/client_service.go

+
+	cacheKey := url
+
+	passthroughFunc := func(item interface{}) (interface{}, error) {


i do not understand what the benefit of the passthroughFunc is giving, could you please elaborate the intent of this?

akinross · 2026-02-23T09:54:54Z

client/client_service.go

+		c.Cache.LogEvent(resourceType+"_CACHED", url, true)
+	} else {
+		// Data was cached by another goroutine while we were fetching
+		log.Printf("[DEBUG] %s already cached by another goroutine for %s", resourceType, url)


should we also then check that the content of the cached item is the same? what happens when they are different?

akinross · 2026-02-23T09:56:50Z

README.md

+// Fetch data with caching support (automatically handles cache hits/misses)
+data, err := msoClient.GetViaURLWithCache("/api/v1/schemas/schema-id")
+
+// Invalidate a specific URL from cache (e.g., after updates)
+msoClient.InvalidateURLCache("/api/v1/schemas/schema-id")
+
+// Clear all cached items (useful for bulk operations or cleanup)
+msoClient.ClearCache()


how do you propose to get the changes into the provider? like after this caching change has been added to the client, what is required in the provider to change in order to support it?

akinross · 2026-02-23T10:00:52Z

client/client_service.go


 }

+func (c *Client) GetViaURLWithCache(url string) (*container.Container, error) {


do we need this a a separate function? why not leverage the GetViaURL and detect if there is caching enabled there, this way none of the current provider code needs to be changed, you would keep functionality as is and then when somebody enables caching it would run that cache function.

add support for caching

24df157

juchowan requested review from akinross, anvitha-jain, gmicol, lhercot, sajagana, samiib and shrsr as code owners December 17, 2025 16:55

akinross requested changes Dec 18, 2025

View reviewed changes

updates based on comments

8724177

juchowan marked this pull request as draft December 22, 2025 15:07

juchowan added 4 commits December 22, 2025 16:57

update tests

658602b

updates to stats and generic caching

1840413

update readme

08d1e5b

fix parallel caching

391edc0

juchowan marked this pull request as ready for review December 23, 2025 13:06

juchowan requested a review from akinross February 10, 2026 14:08

akinross added the jira-sync label Feb 10, 2026

github-actions bot changed the title ~~add support for caching~~ add support for caching (DCNE-680) Feb 10, 2026

akinross requested changes Feb 23, 2026

View reviewed changes


		cacheKey := url

		passthroughFunc := func(item interface{}) (interface{}, error) {


		}

		func (c Client) GetViaURLWithCache(url string) (container.Container, error) {

Conversation

juchowan commented Dec 17, 2025

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!

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!

akinross Dec 19, 2025 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

akinross Dec 19, 2025 •

edited

Loading