Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Issif
Copy link

@Issif Issif commented Jun 29, 2022

For my project, falcosidekick-ui I use redisearch 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 the Document structure with a new field TTL and created a method SetTTL. I tested my fork in my program, and it works

127.0.0.1:6379> keys *
1) "event:1656512211824943"
2) "event:1656512211824021"
127.0.0.1:6379> TTL event:1656512211824021
(integer) 91
127.0.0.1:6379> TTL event:1656512211824021
(integer) 85

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 {
Copy link
Contributor

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.

Copy link
Author

@Issif Issif Jul 13, 2022

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

Copy link
Contributor

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.

Copy link
Author

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

Copy link
Author

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up

@sonarcloud
Copy link

sonarcloud bot commented Jul 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Issif
Copy link
Author

Issif commented Oct 19, 2022

what's the status? thanks

@Issif
Copy link
Author

Issif commented Jan 12, 2023

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?

Copy link

sonarcloud bot commented Apr 13, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants