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

fix: unblock transactions only if requirements are correct #2345

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

BorysTheDev
Copy link
Contributor

fixes #2294

bug: we unblocked XREADGROUP cmd even if we don't have new values

fix: added check with custom requirements for blocking commands

do {
WatchItem& wi = queue.front();
Transaction* head = wi.get();
for (auto wi_it = queue.begin(), wi_e = queue.end(); wi_it != wi_e; ++wi_it) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

as I said before, I would like to preserve a simple and efficient case of lists, sorted sets notifying a single waiter and stop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't change the behaviour)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't get your message at first. let's check how this implementation works. I can implement your suggestion regarding different queues for list, zset and stream like an optimization,

@BorysTheDev BorysTheDev force-pushed the fix_blocking_transactions branch 2 times, most recently from 489bfd9 to 54b891b Compare December 27, 2023 19:19
@BorysTheDev BorysTheDev requested a review from romange December 27, 2023 19:23
@BorysTheDev BorysTheDev marked this pull request as ready for review December 27, 2023 19:23
dranikpg
dranikpg previously approved these changes Dec 29, 2023
Copy link
Contributor

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

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

I approve the approach, just some style comments 🙂

src/server/blocking_controller.cc Outdated Show resolved Hide resolved
src/server/blocking_controller.cc Outdated Show resolved Hide resolved
src/server/container_utils.cc Outdated Show resolved Hide resolved
src/server/transaction.h Outdated Show resolved Hide resolved
dranikpg
dranikpg previously approved these changes Dec 30, 2023
src/server/list_family.cc Outdated Show resolved Hide resolved
Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

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

looks good

fixes #2294

bug: we unblock XREADGROUP cmd even if we don't have new values

fix: added check with custom requirements for blocking comands
@BorysTheDev BorysTheDev force-pushed the fix_blocking_transactions branch from 72976f7 to d368aed Compare January 2, 2024 12:02
@BorysTheDev BorysTheDev merged commit 5b90545 into main Jan 2, 2024
10 checks passed
@BorysTheDev BorysTheDev deleted the fix_blocking_transactions branch January 2, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redis STREAM consumer group have duplicates
3 participants