-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Adds support for XPENDING IDLE parameter #2822
base: main
Are you sure you want to change the base?
Adds support for XPENDING IDLE parameter #2822
Conversation
/// <param name="flags">The flags to use for this operation.</param> | ||
/// <returns>An instance of <see cref="StreamPendingMessageInfo"/> for each pending message.</returns> | ||
/// <remarks>Equivalent of calling XPENDING key group start-id end-id count consumer-name.</remarks> | ||
/// <remarks><seealso href="https://redis.io/commands/xpending"/></remarks> | ||
StreamPendingMessageInfo[] StreamPendingMessages(RedisKey key, RedisValue groupName, int count, RedisValue consumerName, RedisValue? minId = null, RedisValue? maxId = null, CommandFlags flags = CommandFlags.None); | ||
StreamPendingMessageInfo[] StreamPendingMessages(RedisKey key, RedisValue groupName, int count, RedisValue consumerName, RedisValue? minId = null, RedisValue? maxId = null, long? minIdleTimeInMs = null, CommandFlags flags = CommandFlags.None); |
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.
we need to add an overload here, i.e. a second method with different parameters; otherwise, this is a hard binary break - we try very hard not to do that. If the compiler complains about two methods with optional parameters, we can work around that
(yes: technically adding methods to the interface is also problematic, but: it is problematic in different ways, and in reality we don't expect custom implementations of the IDatabase
etc APIs)
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.
+1 on this. I'd leave the existing public method signatures as-is, and add new methods with additional required params as needed.
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.
Do you have a suggestion on the overloads? This Redis API can satisfy a number of use cases depending on the parameters passed. minIdleTimeInMs can be used with various combinations of consumerName, minId, and maxId. If new overloads do not have defaults, several overloads would likely be needed to satisfy all use cases which complicates the public API.
With this many arguments I'd generally lean toward an class like StreamPendingMessagesArgs, but that seems to run afoul of the existing design.
The PR as it currently exists seemed like a reasonable compromise between backwards compatibility and adhering to the design.
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 was thinking something like this (but @mgravell and @NickCraver are really the experts):
StreamPendingMessageInfo[] StreamPendingMessages(RedisKey key, RedisValue groupName, int count, RedisValue consumerName, long minIdleTimeInMs = null, RedisValue? minId = null, RedisValue? maxId = null, CommandFlags flags = CommandFlags.None);
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.
@mgravell, would you like me to amend the PR to restore the existing overload and add a new one that places minIdleTimeInMs before min/maxId?
var offset = 0; | ||
|
||
values[offset++] = groupName; | ||
if (minIdleTimeInMs is not null) |
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.
👍
@@ -1154,8 +1154,8 @@ public void StreamPendingInfoGet() | |||
[Fact] | |||
public void StreamPendingMessageInfoGet() | |||
{ | |||
prefixed.StreamPendingMessages("key", "group", 10, RedisValue.Null, "-", "+", CommandFlags.None); | |||
mock.Received().StreamPendingMessages("prefix:key", "group", 10, RedisValue.Null, "-", "+", CommandFlags.None); | |||
prefixed.StreamPendingMessages("key", "group", 10, RedisValue.Null, "-", "+", 1000, CommandFlags.None); |
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.
if this 1000
is the new arg, we probably need a version-check here, to not break test setups
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 following the concern here and likely showing my lack of knowledge about your setup. 1000 is the new arg, but this seems to be mocked and I'm not following how that could break. What is the "test setup" you're referencing?
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.
oh, if it is mocked: fine - ignore me
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.
main problem here is the API break - we need to do that differently (see notes)
also: please add to releasenotes.md
- Add release notes - Add overload to StreamPendingMessages/StreamPendingMessagesAsync to preserve back compat
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.
technically this is a little weird if somebody was passing literal 0
as the flags
parameter on the old API, which is legal but unusual; I'm OK with it, though - any thoughts from @NickCraver @philon-msft ?
@mgravell, how about having a |
I preferred to mirror the existing APIs around streams. All use a long. https://github.com/search?q=repo%3AStackExchange%2FStackExchange.Redis%20idleTimeInMs&type=code |
@NickCraver @philon-msft, bumping in case this was overlooked during year end. |
Adds support for Idle time filter on XPENDING command.
This filter is only available on Redis 6.2+, but I didn't see an indication that version is checked when building command arguments/values.
CommandFlags appeared always to be the last method argument. Adding minIdleTimeInMs before the flags seems like a breaking change to the signature of StreamPendingMessages/StreamPendingMessagesAsync. I can amend if desired.
fixes #2432