-
Notifications
You must be signed in to change notification settings - Fork 102
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 semaphore-like permits for some actor operations #906
Conversation
No I don't think so - the only code in the actor that pushes requests to the actor is for offset commits, which aren't themselves invoked via the actor. |
Oops, I broke something somewhere along the line 😬 |
I checked these changes briefly. They may be good by itself, but I talked about a different approach: I wanted to completely get rid of a To create polling we could run a fiber that uses the same semaphore and the same java consumer and run this fiber in the background. |
I agree that would be preferable as an end state, but I think the changes in this PR make direct progress towards it: the code that calls One thing about using a single semaphore that may be an issue is that it would be strictly FIFO rather than the current approach that distinguishes polls from other requests - that said, I’m not sure the current approach is actually correct. What I thought we were currently doing, and what seems right, is to always handle all pending requests before doing a poll, but what actually seems to happen is that once the request queue is drained the actor will block until the next poll, and only after the next poll will it process requests that happened after the queue was drained. @vlovgr can you clarify what the intended behaviour is? |
Ideally, we should process all fetch requests before polling, since partitions without fetch requests will remain paused when polling (as you note, we aren't really doing exactly that today). If it makes a difference in practice versus just processing everything FIFO is difficult to say. The only contract we really have to adhere to is to ensure poll is called at least every |
Is there any objection to merging these changes as a step towards using a semaphore for everything? |
This was supposed to be a demo but I got a bit carried away - inspired by #903