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

SADDEX option to update an existing member's TTL #4676

Closed
jgaskins opened this issue Mar 1, 2025 · 4 comments · Fixed by #4700
Closed

SADDEX option to update an existing member's TTL #4676

jgaskins opened this issue Mar 1, 2025 · 4 comments · Fixed by #4700
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone

Comments

@jgaskins
Copy link

jgaskins commented Mar 1, 2025

Did you search GitHub Issues and GitHub Discussions First?
Yes, there is only one open issue for SADDEX.

Is your feature request related to a problem? Please describe.
I have a use case for upserting a member to a set with an expiration and applying that expiration regardless of whether the set already has that member — basically refreshing the TTL each time the member is upserted.

I am currently working around it by removing and reinserting it in a MULTI block:

MULTI
SREM set member
SADDEX set 30 member
EXEC

But ideally this could be atomic with a single command.

Describe the solution you'd like

I think an option like this might work:

SADDEX REFRESH 30 foo bar baz

I'm not too concerned about what the option is called. I'm just calling it REFRESH at the moment because I can't come up with anything else.

Describe alternatives you've considered

Originally, I wanted it after the members' TTL:

SADDEX 30 REFRESH foo bar baz

But if we do that, REFRESH being an option or a value to add to the set is ambiguous. Either the option doesn't do anything or you can't add a member called "REFRESH" to a set via SADDEX. I feel like it has to come before the TTL because it must be an integer, so if the server sees SADDEX REFRESH <integer> member ... there is no ambiguity.

I feel like REFRESH (or whatever the option ends up being called) must come before the TTL so there is no ambiguity.

Additional context

My use case is that SADDEX is an important part of ingesting time series data. The time series are stored in streams, and each time series represents a set of label=value pairs. I don't want the references to the streams to expire with data in those streams, so I need the TTL to be updated every time I record a measurement.

Since I'm currently working around it with SREM + SADDEX, if this option is added ingesting metrics will only require about half as many commands sent to the server, allowing close to double the ingestion rate for a given set of label=value pairs.

@adiholden adiholden added good first issue Good for newcomers enhancement New feature or request labels Mar 2, 2025
@romange
Copy link
Collaborator

romange commented Mar 2, 2025

Have you looked at https://www.dragonflydb.io/docs/command-reference/generic/fieldexpire @jgaskins ?

would it help?

@jgaskins
Copy link
Author

jgaskins commented Mar 2, 2025

I did forget about that one. It would save me from having to remove members from the set, but wouldn't avoid the need for a second command per label/value pair. This is possible with HSETEX but not SADDEX, so adding this feature would put them on par in that way.

I did consider using hashes instead of sets for this for that reason, but assumed sets would have a smaller footprint.

@romange
Copy link
Collaborator

romange commented Mar 2, 2025

what is is the "label" and what is the "value" in your SADDEX set 30 member command?

I actually was not aware of this undocumented difference in the behavior between hsetex and setex and I think I understand now the semantics you ask for. I think it makes sense.

For the protocol, it's something like this in c++ pseudo code:

auto [it, added] = set->insert(member, ttl);
if (!added)
  it->ttl = ttl;

Here is what I suggest:

  1. We should fix the behaviour so that both commands by default will update the TTL for the existing member.
  2. We should introduce a runtime flag legacy_saddex_keepttl that should be by default false but if true then Dragonfly reverts to the previous behavior for SADDEX. The flag should retire, say around Oct 2025 to give enough time for folks to migrate.
  3. We should introduce an optional KEEPTTL that DOES not change the ttl for the existing members. I find it more consistent with SET. This option should be present for both SADDEX and HSETEX commands.

@jgaskins what do you think?

@jgaskins
Copy link
Author

jgaskins commented Mar 2, 2025

what is is the "label" and what is the "value" in your SADDEX set 30 member command?

Apologies, the set keys are an index of label/value pairs. So for example, tsref:http.requests:response:500 contains the stream keys for each time series that tracks all of the http.requests metrics for which the response label's value is 500. tsref = "time series reference".

So if I'm ingesting a metric that has the normalized labels response=500,response_class=server_error, it makes these calls (others removed for brevity):

XADD ts:http.requests:response=500:response_class=server_error MINID ~ #{retention_cutoff} * value #{value}
SADDEX tsref:http.requests:response:500 #{retention} ts:http.requests:response=500:response_class=server_error
SADDEX tsref:http.requests:response_class:server_error #{retention} ts:http.requests:response=500:response_class=server_error

I actually was not aware of this undocumented difference in the behavior between hsetex and setex

It surprised me, too, when I saw the set members expiring despite metrics for those time series coming in steadily.

For the protocol, it's something like this in c++ pseudo code:

auto [it, added] = set->insert(member, ttl);
if (!added)
  it->ttl = ttl;

Yes, this is pretty much exactly what I need. 💯

Here is what I suggest:

  1. We should fix the behaviour so that both commands by default will update the TTL for the existing member.
  2. We should introduce a runtime flag legacy_saddex_keepttl that should be by default false but if true then Dragonfly reverts to the previous behavior for SADDEX. The flag should retire, say around Oct 2025 to give enough time for folks to migrate.
  3. We should introduce an optional KEEPTTL that DOES not change the ttl for the existing members. I find it more consistent with SET. This option should be present for both SADDEX and HSETEX commands.

@jgaskins what do you think?

I really like this plan. The synergy with SET's KEEPTTL option is a great choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants