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

Python: add XGROUP SETID command #1683

Merged

Conversation

aaron-congo
Copy link
Collaborator

@aaron-congo aaron-congo commented Jun 26, 2024

Issue #, if available:
N/A

Description of changes:
https://redis.io/docs/latest/commands/xgroup-setid/

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aaron-congo aaron-congo requested a review from a team as a code owner June 26, 2024 23:45
@aaron-congo aaron-congo force-pushed the python/integ_acongo_xgroup_setid branch from e9d2dac to 52263e1 Compare June 27, 2024 00:11
@aaron-congo aaron-congo changed the title Python/integ acongo xgroup setid Python: add XGROUP SETID command Jun 27, 2024
Comment on lines +2936 to +2938
entries_read_id (Optional[str]): An arbitrary ID (that isn't the first ID, last ID, or the zero ID ("0-0"))
used to find out how many entries are between the arbitrary ID (excluding it) and the stream's last
entry. This argument can only be specified if you are using Redis version 7.0.0 or above.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As more I read this, as less I understand it.
No complains to you, you just copied the doc. Do you understand how it changes the command behavior?

Copy link
Collaborator Author

@aaron-congo aaron-congo Jun 27, 2024

Choose a reason for hiding this comment

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

Yeah this is pretty hard to understand from the existing Redis docs. I think this is relevant to the XINFO GROUPS command. If I understand correctly, you can pass this to tell Redis what the last entry received was for the consumer group. Then when you execute XINFO GROUPS it can return this info to you, as well as a count of how many messages have arrived since the indicated entries_read_id

# reset the last delivered ID for the consumer group to "1-1"
# ENTRIESREAD is only supported in Redis version 7.0.0 and above
if await check_if_server_version_lt(redis_client, "7.0.0"):
assert await redis_client.xgroup_set_id(key, group_name, stream_id1_1) == OK
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a separate command to call to test that ENTRIESREAD has an affects the group?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think XINFO GROUPS, we can probably test this once that gets implemented

# XGROUP SETID accepts "0" for the entries read ID, but does not accept "0-0"
with pytest.raises(RequestError):
await redis_client.xgroup_set_id(
key, group_name, stream_id1_1, entries_read_id="0-0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe change this:

Suggested change
key, group_name, stream_id1_1, entries_read_id="0-0"
key, group_name, stream_id1_1, entries_read_id="stream_id0"

since the documentation says it cannot be the first or zero entry.

Copy link
Collaborator Author

@aaron-congo aaron-congo Jun 27, 2024

Choose a reason for hiding this comment

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

stream_id0 is equal to "0". Oddly enough, "0" is accepted for the entries_read_id but "0-0" is not. Strangely, "1-1" is not accepted either, even though it is not the first, last, or zero ID. It seems as though it only accepts IDs that don't have a dash. I tested this with Redis CLI as well. I'll just change this to stream_id1_0 for now since it is the first entry, even though it also won't accept "1-1"

@aaron-congo aaron-congo force-pushed the python/integ_acongo_xgroup_setid branch from c80ea9d to 7e5d9d6 Compare June 27, 2024 16:54
@acarbonetto acarbonetto merged commit 20ba73b into valkey-io:main Jun 27, 2024
46 checks passed
@acarbonetto acarbonetto deleted the python/integ_acongo_xgroup_setid branch June 27, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants