[GitHub] [pulsar] equanz opened a new pull request #14981: [websocket] Replace blocking logic to avoid OOM

[GitBox <gi...@apache.org>]

equanz opened a new pull request #14981:
URL: https://github.com/apache/pulsar/pull/14981


   ### Motivation
   
   I encountered a OOM issue in pulsar-websocket because user do negative ack very frequently for mistaken.
   For the heap dump, many `org.apache.pulsar.websocket.ConsumerHandler$1` are referenced from ConsumerHandler.
   https://github.com/apache/pulsar/blob/acac76e9799bc31e388c593aa553711c6ad734aa/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ConsumerHandler.java#L169-L187
   
   I'd like to fix the issue.
   
   ### Modifications
   
   I added the new configuration param `webSocketMaxOutgoingFrames` to use jetty's `MaxOutgoingFrames` feature.
   https://github.com/eclipse/jetty.project/commit/71df3b57eef08040203337f6d83ff28adc86d7c7
   We can avoid increasing of pending outgoing frames.
   If the outgoingFrames is reached to max and try to send, jetty calls writeFailed of new sending callback.
   
   However, ConsumerHandler$1#writeFailed calls ScheduledExecutorService#execute without any limitation.
   Thus, this approach alone can't decrease the number of ConsumerHandler$1 instances.
   https://github.com/apache/pulsar/blob/acac76e9799bc31e388c593aa553711c6ad734aa/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/ConsumerHandler.java#L176
   
   I replaced pendingMessages feature from counting number of requests to counting actual message id.
   Because, ConsumerHandler decrements pendingMessages even if user requests negative ack by same message id.
   Also, I replaced ConsumerHandler$1#writeFailed to call ScheduledExecutorService#execute when no more ConsumerHandler#receiveMessage is executed.
   
   ### Verifying this change
   
   - [ ] Make sure that the change passes the CI checks.
   
   This change added tests and can be verified as follows:
   
   - Added unit tests for ConsumerHandler
   
   ### Does this pull request potentially affect one of the following parts:
   
   - Dependencies (does it add or upgrade a dependency): (yes)
     - in test scope
   - The public API: (no)
   - The schema: (no)
   - The default values of configurations: (yes)
   - The wire protocol: (no)
   - The rest endpoints: (no)
   - The admin cli options: (no)
   - Anything that affects deployment: (no)
   
   ### Documentation
   
   Check the box below or label this PR directly (if you have committer privilege).
   
   Need to update docs? 
   
   - [x] `doc` 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org