-
Notifications
You must be signed in to change notification settings - Fork 66
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
add new command EXPIRE #167
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Issif <[email protected]>
@@ -70,6 +71,12 @@ func (d Document) Set(name string, value interface{}) Document { | |||
return d | |||
} | |||
|
|||
// SetTTL sets the ttl in the document | |||
func (d Document) SetTTL(ttl int) Document { |
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 can definitely see the value... But perhaps we should at least set a default TTL of -1 during the New.
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.
Without the set, it's by default to -1
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.
Correct - I just prefer being explicit.
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 in holidays, I'll update this PR when I'm back
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 thought and I think we should let the code like this, in this current setup, if the TTL is not set, we follow the Redis default behavior and the keys have no expiration, if we set the TTL, a second command is run to add an expiration to the key. It means, if we don't need to set the TTL we run only 1 command and not 2. For huge workload, it can be useful to avoid to run 2 commands when it's not necessary. wdyt?
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.
Up
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
what's the status? thanks |
Hi, My project is using my fork but I would like to plug it on the upstream, what can I do to move forward with this PR please? |
Quality Gate passedIssues Measures |
For my project,
falcosidekick-ui
I useredisearch
as storage for the events, it works really well but the community asked me to add a TTL to keys. It's not currently possible in this SDK, so updated theDocument
structure with a new fieldTTL
and created a methodSetTTL
. I tested my fork in my program, and it worksSigned-off-by: Issif [email protected]