-
Notifications
You must be signed in to change notification settings - Fork 352
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
Add ability to configure the queue capacity for ChunkedOutput #5621
Add ability to configure the queue capacity for ChunkedOutput #5621
Conversation
Adding constructors which sets the queue capacity. This allows for calling write from different threads until the queue is full. Subsequent write calls will block until the queue is emptied. This change allows to prevent memory issues in case of slow clients. Basically, a backpressure mechanism based on the queue size. This change provides a way to call write on ChunkedOutput from multiple threads to improve throughput, while preventing memory issues in case of slow clients. The queue is bounded and thus cannot grow endlessly.
Update year
@rutterpaul-personal @paulrutter This PR just adds 6 new constructors. Would it be better to have a builder instead? |
@jansupol
That could work for me. We recently ran into memory issues because we no longer synchronized on calling the |
Yes, I'd keep the constructors as they are, for the backward compatibility, without adding any new (except for the one accepting the builder, setting up all the values set on the builder to the ChunkedOutput). |
Ok, i will work on that instead. |
Something like this then?
I cannot get the @jansupol what do you think about the chunkType? Can i drop it from the builder? I noticed that the |
Use builder instead.
Copyright
Add chunkType, String by default.
Add javadoc
Hide constructor
If these changes are ok, documentation probably needs updating on https://eclipse-ee4j.github.io/jersey.github.io/documentation/2.42/async.html#chunked-output and it would need a unit test to test the builder. |
Add e2e test that uses builder with queue capacity of 2.
copyright
Update documentation with an example of the builder with the capacity
Copyright
Make chunkType a required argument to the builder. Change builder methods.
Update doc.
Update test
Fix checkstyle issue
The builder looks good. I'd prefer the name The chunkType is required to know the type in runtime, or the type can be known from the ChunkedOutput subclass (hence the protected constructors), the same way the GenericType works. So, with the protected constructor functionality, If needed, I think the builder could work with constructs like:
wdyt? |
@jansupol That would make sense, except that i cannot call
Or do i misunderstand? |
@paulrutter My bad, it's not that simple. Would need to have two constructors for that... And two builders... Maybe something like
Perhaps too complicated at the moment ... unless you would have a use case for it. |
Add two builders, one with and one without type.
Update test
Update docs
Any idea in which version this will land? |
2.43 the next week hopefully |
I assume it will also land in 3.x? |
Sure, 3.0 right after 2.43, 3.1 a bit later, 4.0 next. |
Confirmed it works on 2.43. |
Adding constructors which set the queue capacity.
This allows for calling
ChunkedOutput.write
from multiple concurrent threads until the queue is full. Subsequentwrite
calls will block until the queue is emptied byflushQueue
. This change allows to prevent memory issues in case of slow clients. Basically, a backpressure mechanism based on the queue size.This change provides a way to call write on
ChunkedOutput
from multiple threads to improve throughput, while preventing memory issues in case of slow clients. The queue is bounded and thus cannot grow endlessly.The code now uses
put
(https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/LinkedBlockingQueue.html#put-E-) instead ofadd
(https://docs.oracle.com/javase/8/docs/api/java/util/AbstractQueue.html#add-E-) so it blocks when the queue is full.