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 support for stream XTRIM MINID command #1718

Open
WeihanLi opened this issue Apr 16, 2021 · 10 comments · May be fixed by #2842
Open

Add support for stream XTRIM MINID command #1718

WeihanLi opened this issue Apr 16, 2021 · 10 comments · May be fixed by #2842

Comments

@WeihanLi
Copy link
Contributor

From the redis command document, I learned about that we could use XTRIM mystream MINID 649085820 to implement all entries that have an ID lower than 649085820-0 will be evicted, so I'm wondering if we could add support for MINID subcommand for stream XTRIM.

Besides, I noticed that it seemed we could use MINID subcommand when we try to XADD new entry to a stream, maybe we could have it implemented?

@WeihanLi
Copy link
Contributor Author

I think maybe it's better to add a new method to avoid breaking changes, for example:

For XTRIM:

StreamTrimWithMinId(RedisKey key, RedisValue minId, bool useApproximateMaxLength = false, CommandFlags flags = CommandFlags.None);

For XADD:

RedisValue StreamAddWithMinId(RedisKey key, RedisValue streamField, RedisValue streamValue, RedisValue minMessageId, RedisValue? messageId = null, bool useApproximateMaxLength = false, CommandFlags flags = CommandFlags.None);

RedisValue StreamAddWithMinId(RedisKey key, NameValueEntry[] streamPairs, RedisValue minMessageId, RedisValue? messageId = null, bool useApproximateMaxLength = false, CommandFlags flags = CommandFlags.None);

@WeihanLi
Copy link
Contributor Author

WeihanLi commented Aug 24, 2021

Hi @NickCraver, @mgravell, any thoughts on this?

If we could accept breaking changes, I think maybe it's better to add support for a trim policy, maybe something like follows:

public enum StreamTrimPolicy
{
  MaxLength = 0,
  MinID = 1,
}

RedisValue StreamAdd(RedisKey key, RedisValue streamField, RedisValue streamValue, StreamTrimPolicy trimPolicy = StreamTrimPolicy.MaxLength , RedisValue? minMessageIdOrStreamLength = null, RedisValue? messageId = null, bool useApproximateMaxLength = false, CommandFlags flags = CommandFlags.None);

long StreamTrim(RedisKey key, StreamTrimPolicy trimPolicy = StreamTrimPolicy.MaxLength , RedisValue minMessageIdOrStreamLength, bool useApproximateMaxLength = false, CommandFlags flags = CommandFlags.None);

@mgravell
Copy link
Collaborator

mgravell commented Aug 24, 2021

We try very very hard to avoid MME problems, so unless we already have a necessary "major" (break), it is preferred to do this in a way that doesn't break compatibility. Adding overloads is fine, though. The usual approach is to remove the optional parameters from the old method (making them mandatory parameters), which means new builds prefer the new methods (except for exact matches)

@WeihanLi
Copy link
Contributor Author

Thanks @mgravell , so it's suggested to add new methods to implement this, right? If so, how about the new method signature above.
If it's fine, maybe I could contribute to this.

@morinow
Copy link

morinow commented Feb 2, 2022

Any update on this so far? Would be awesome to have this.

@WeihanLi
Copy link
Contributor Author

WeihanLi commented Feb 5, 2022

Any more thoughts on this @NickCraver @mgravell

@qrli
Copy link

qrli commented Aug 18, 2022

I find myself in need of this feature. Before it is added, is there any workaround?

@WeihanLi
Copy link
Contributor Author

Hi @qrli , you can use the Execute/ExecuteAsync method to execute a command not supported by a specific API.
For example:

// https://redis.io/commands/xadd, https://redis.io/commands/xtrim
var redis = ConnectionMultiplexer.Connect("").GetDatabase();
var result = redis.Execute("XADD", "stream-key", "MINID", "=",
     DateTimeOffset.UtcNow.Subtract(Expiry).ToUnixTimeMilliseconds(), "*", "message", "stream-message");
var msgId = result.ToString();

@kijanawoodard
Copy link

Is anyone working on this one? I might pick it up over the holiday.

@kijanawoodard kijanawoodard linked a pull request Jan 21, 2025 that will close this issue
@kijanawoodard
Copy link

I didn't get to it over the holidays, but I got the PR in.
Have a few question, but maybe it's reasonably close?

#2842

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

Successfully merging a pull request may close this issue.

6 participants