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

Use assign/pause/redirect/unpause pattern #111

Merged
merged 2 commits into from Oct 21, 2019
Merged

Conversation

AlexeyRaga
Copy link
Member

Changes

Questions:

@shlevy do you think this may work?

@AlexeyRaga
Copy link
Member Author

I am still very skeptical about this because I don't see how it can be guaranteed that no messages can be delivered into a "default" consumer queue in between of assign and pause.

Or does the fact that we are already in an event loop safe us here? Like, even if some messages have been delivered to the "events" queue, pause will make sure that they are gone and they will be re-delivered to the appropriate queue after unpause happens?

@shlevy
Copy link
Contributor

shlevy commented Oct 7, 2019

@AlexeyRaga Two thoughts:

  • Does assign cause an unpause? If not can't we pause before assign?
  • Maybe we need to pause our poll thread here?

I think we'll need @edenhill to reach any confidence here 😦

@edenhill
Copy link

edenhill commented Oct 7, 2019

If you don't have a separate thread that calls poll() and feeds messages into a local queue then it is perfectly fine to do assign() + pause(), since even if messages were fetched after the assign they will be purged by pause() and there is no poll done between those two calls.

@shlevy
Copy link
Contributor

shlevy commented Oct 7, 2019

OK, so in that case @AlexeyRaga I think we need to do something like:

  1. Have pollConsumerEvents take an mvar before polling and restore it afterward
  2. Take the mvar in assignmennt
  3. assign
  4. pause
  5. redirect
  6. resume
  7. restore mvar

@AlexeyRaga AlexeyRaga force-pushed the rebalance-pause branch 3 times, most recently from 1e29a81 to cbfc186 Compare October 11, 2019 03:19
@AlexeyRaga
Copy link
Member Author

I now use an MVar to make sure that only one thread can poll for events/callbacks at the time.
I was able to finally remove that stupid thread delay and I cannot reproduce any jumps or double-dispatches.

@shlevy it would be great to check that for that double-dispatch issue that you had against this version.

@AlexeyRaga AlexeyRaga force-pushed the rebalance-pause branch 3 times, most recently from fd3de47 to 106ec25 Compare October 11, 2019 05:08
@shlevy
Copy link
Contributor

shlevy commented Oct 13, 2019

@AlexeyRaga Sounds good, I'll give this a shot this week.

@AlexeyRaga AlexeyRaga merged commit 5901585 into master Oct 21, 2019
@AlexeyRaga AlexeyRaga deleted the rebalance-pause branch October 21, 2019 03:12
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.

None yet

3 participants