-
Notifications
You must be signed in to change notification settings - Fork 326
[Streams-adapters]: Cache, helpers, types #4318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Streams-adapters]: Cache, helpers, types #4318
Conversation
|
ro-tex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far! I like the method docstrings.
We'll need some tests at some point but it can be further down the line.
| // Items returns all items in the cache | ||
| func (c *Cache) Items() map[string]*types.CacheItem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the use case for this method is.
Are we OK with returning pointers to the actual data? OTOH, if we don't return pointers we might run OOM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case for this is a debug API endpoint (/debug/cache/items) that returns the entire cache.
@brunotm need your opinion on pointers.
|
I am not sure if this is the correct repo |
This is the correct repo. Design document: https://docs.google.com/document/d/18opUJqvmrgQjy5UHjmEnwv22zdSJOre4o7-wnk3trqs/edit?tab=t.0#heading=h.b8hxx51vbzqg |
Happy to chat about this in DMs, as far as repo placement this seems like an easy place but I could also see it hosted somewhere else entirely, don't think we went that deep in our high level solution discussions |
I think prob better to be in a brand new repo? |
ro-tex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| if err != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how helpful logging the error would be but I wanted to flag it as an idea. Right now this will error out and we won't know why.
Closes #DS-1422
Description
Initial commit with cache implementation, with required types, and helper functions.