-
Notifications
You must be signed in to change notification settings - Fork 310
fix: Recreate group in case redis flush #2656
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
base: main
Are you sure you want to change the base?
fix: Recreate group in case redis flush #2656
Conversation
|
I added all the casting to pass the linter. Also, I discivered that this part is not tested at all. (if I insert an exit in that part of the code the tests are running fine. If you have some test example, I would imagine something such as: create stream with group |
| ) | ||
| except ResponseError as e: | ||
| if "NOGROUP" in str(e): | ||
| await self._create_group() |
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.
Should we change current last_id read position at stream recreation?
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.
indeed we should. The group being reseted, next id should retart from scratch.
I added some tests as well to be sure it was working better
befd241 to
b70b55f
Compare
|
My code is entering into conflict with the one from 9998c58 I have no idea what the fix on autoclaim is, so it's very hard to understand how to mix it with my code and see what should happen. May you help please ? @JonathanSerafini |
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.
not sure if this was intended, but your redis PR also includes this change and an unrelated package version change.
| ..., | ||
| ], | ||
| ]: | ||
| async def read_from_group() -> tuple[ |
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 thinking at this point you may as well just make def read an async function and do without the nested read_from_group it may make things more readable ?
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.
this type annotation is likely the first conflict, you can use ReadResponse as a replacement
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.
the other conflict probably stems from the fact that there are now 2 stream group readers in this code now, one for xreadgroup and another for xautoclaim ... i would assuming that if you want to re-create the group for the former you may also want for the latter.
for the xautoclaim handling, i would just start over from 0-0 i.e the first position and it should be able to take it from there.
- adding a test - reset properly the id - adding another rare case when group is tempered by another system, and IDs gets desynchronized. (AI recommendation)
b70b55f to
cad2597
Compare
- fixing group creation in autoclaim + get_one message and iter.
- fixing group creation in autoclaim + get_one message and iter.
|
Thanks for you feedback. Thanks to it, I decided to crete a specific function that has to protect any provided method against group removal. I updated the code accordingly. Tests are passing. I still have the collowing linter error, but I don't know how to make them disapeer. I'm totally lost in the return typing used to be honnest. Any idea on how to correct that ? |
|
@JonathanSerafini Would you think it' possible to merge it ? Last time it was working fine, but the more the system progress, the more I have to redo the job... |
|
@pbonneaudiabolocom I'm not the right one to ask, i'm not a maintainer or affiliated with this project. That being said re:typing, you have to account for the changes you're making. You've created To account for different input functions you might want to look at how to type a 'parametrized decorator'. |
Description
When using redis with group, if a flush is done by another parties or the stream is destroyed, faststream enter in an infinite loop, trying to get the next element but not finding the expected group.
Fixes #2611
Type of change
Please delete options that are not relevant.
Checklist
just lintshows no errors)just test-coveragejust static-analysis