-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Replace dgraph-io/badger cache storage with etcd-io/bbolt #42571
base: main
Are you sure you want to change the base?
Conversation
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
This pull request doesn't have a |
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
Co-authored-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
…to drop-dbadger-io
@@ -97,7 +104,7 @@ func (c *PersistentCache) Get(k string, v interface{}) error { | |||
return err | |||
} | |||
if c.refreshOnAccess && c.timeout > 0 { | |||
c.store.Set([]byte(k), d, c.timeout) | |||
_ = c.store.Set([]byte(k), d, c.timeout) |
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.
[Suggestion]
Shouldn't the error be logged?
I see the original code also ignored the error, but I wonder if it's the best approach.
I know it's a cache and by definition it should not be a fatal error not being able to set a value in the cache, but if for some reason it keeps happening, it might impact the overall performance of whatever is using the cache. Besides if it fails often, there might be an issue that needs to be investigated.
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, I wasn't intending to change the code (had to due to linter complains). Do you want me to add logging of the error?
} | ||
|
||
// Connect creates directories of a given path for bbolt DB file (if directories not already exist), creates DB file with given file permissions, creates bucket to store cache data. | ||
func (b *Bbolt) Connect() error { |
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.
[NIT]
It's not connecting really connecting somewhere, perhaps Open
is more descriptive as it operates on files
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.
yes but the name comes from interface which is intended as abstraction (bbolt is just an implementation, another implementation maight be redis or or nats.io something different). At least this was my reasoning...
Proposed commit message
Replacing dgraph-io/badger persistent storage for key-value cache with etcd-io/bbolt. Originally it was meant to just get rid of
go.opencensus.io
dependency which is introduced by badger (please see parent issue for more details). After it got evident that this won't erasego.opencensus.io
dependency it was decided that this work still should be done since etcd-io/bbolt is already used elsewhere in the project and it isn't a good thing to have multiple storages for cache (again, please see parent issue for more details (in comments)).Implementation should be fairly straight-forward but I would like to clarify one thing - since bolt doesn't support value expiration the expiration time (and TTL) are stored as metadata of the value. Upon value retrieval it is checked for expiration and if it is expired then nil is returned and value gets deleted from bolt DB.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Disruptive User Impact
Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs