[GitHub] [pulsar] lhotari commented on pull request #10944: [Proxy] Limit replay buffer size in AdminProxyHandler

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

lhotari commented on pull request #10944:
URL: https://github.com/apache/pulsar/pull/10944#issuecomment-863490803


   Thanks for the great suggestions @addisonj .
   
   > I wonder if we need a at least a tweak or a different approach... If there is any place where we have an arbitrary body that has a redirect in the loop, I would be nervous that you would hit this and then get a truncated body on redirect.
   
   Yes a different approach would be useful. It seems just wrong to buffer all request bodies in memory for the duration of the request handling.
   
   > make it tuneable and the user needs to be aware if they are using HTTP endpoints for producing/consuming. Perhaps just setting the default of 5 MB of max message size would cover 90% use case? I think this is the minimum
   
   Makes sense. I could add a configuration option. I thought it wouldn't be so common to have http requests with large bodies and it would be mainly the Pulsar Function jar uploads that could exceed 1MB. I guess with new APIs such as PIP-64, 1MB could exceed when sending large messages.
   
   > do something smarter and use an off heap buffer or some smarter allocation strategy
   
   Allocating on heap isn't the biggest problem. It's the usage of ByteArrayOutputStream which uses a single byte array. 
   I'm the author of StreamByteBuffer/StreamCharBuffer classes which are used in Grails, Gradle and MyFaces for smarter buffering. Essentially the key idea is to use a linked list of smaller byte arrays. For example [StreamByteBuffer in Gradle](https://github.com/gradle/gradle/blob/master/subprojects/base-services/src/main/java/org/gradle/internal/io/StreamByteBuffer.java) / [StreamByteBuffer in Grails](https://github.com/grails/grails-core/blob/master/grails-encoder/src/main/groovy/org/grails/buffer/StreamByteBuffer.java) shows this. Both versions are covered with Apache license and I'm the original author so there shouldn't be a problem in reusing some part of those classes in a StreamByteBuffer for Pulsar. The simplest version wouldn't require a String conversion which is most of the complexity in the above examples.
   
   > make the proxy smarter and if it is a request that has a post body and may be redirected we do a lookup and then always hit the correct broker. This might be a longer term effort
   
   I think this would be a good solution.
   
    
   
   
   
   
   
   
   
   
   


-- 
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.

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