I think that we can deprecate maxRequestKB and maxResponseKB and just set them to the highest possible value that we can and not expose them to the user.

IMO, these may still be useful, but semantics must change.
One of the benefits of the service mesh is that it handles some common communication features for services and lets services be simpler. As a minimum service mesh offers service discovery and load balancing. In the case of linkerd this also includes retries, circuit breaking, etc.. Limiting request and response sizes fits here very well.

However to be useful maxRequestKB and maxResponseKB must work not only for non-chunked (in your terminology, thanks for describing this btw) messages but for the other 2 categories too. This will require some development, likely even a contribution to finagle.

Your idea to remove those is very pragmatic. But there may be existing linkerd users who rely on maxRequestKB and maxResponseKB as a protective measure for their services.

Alternatively we can try to submit a patch to finagle to allow lifting that 2GB threshold higher

+1 for this. I believe, there's no good reason to have 2GiB hard limit in finagle now after support for streaming of fixed-length messages is implemented.

@edio Thanks for your comments. To be completely frank, all of this terminology is quite confusing and not documented very well at all, so it was quite the can of warms to look through that stuff. I agree with you about most what you said and will probably investigate the headers bit as well. I think the relationship will be somewhat similar to what it is between fixedLengthStreamedAfter and maxRequestKB

With respect to making maxRequestKB work with all sorts of messages, I am not sure whether this is easy/possible at all. If you think about it in order to classify a message as being over the size limit you either need to have Content-Length header or buffer it entirely, which makes it a not streamed message... Nevertheless will discuss more with the rest of the team and let you know :)

With respect to making maxRequestKB work with all sorts of messages, I am not sure whether this is easy/possible at all. If you think about it in order to classify a message as being over the size limit you either need to have Content-Length header or buffer it entirely, which makes it a not streamed message... Nevertheless will discuss more with the rest of the team and let you know :)

I may be very off here (and can't check right now), but IIRC this is technically feasible with netty: a handler has a counter which we increment with every frame transferred by the size of the frame. Once the counter reaches the limit we abort the transmission and return 413. Obviously there may be something special about how the handler is added to netty pipeline or how the counter is declared so we count only our own message... I do not recall technicall details, but I do believe this is possible.

I will try to look into it later today.

@edio Yes, you are right, thinking about it it is quite possible to do that in such a way. I am not too familiar with the finagle/netty codebase but sounds like its possible. Will investigate what we can do (and when)

Thanks to @zaharidichev and @edio for all of the investigation you have done on this. It's great to finally get some clarity on the behavior of these parameters.

If I understand correctly, the only time the maxRequestKB parameter has any effect is for non-chunked requests with a Content-Length which is larger than maxRequestKB but smaller than streamAfterContentLengthKB. Is that correct?

If so, that's pretty niche and I agree with @zaharidichev's original suggestion of removing maxRequestKB and maxResponseKB.

The original purpose of these parameters was to protect Linkerd from excessive buffering and memory pressure from large messages. I think the streamAfterContentLengthKB accomplishes this in a much more graceful way.

It is true that having the functionality of having the service mesh limit message sizes could be useful. But that's not functionality we have today (because the maxRequestKB don't limit chucked messages) so I think we should decouple the work of clarifying our existing functionality from the work of adding new functionality.

@edio: if you think limiting message size is a useful feature, would you mind opening an issue with details about how you think it should work?

@adleong at NCBI we do not use this feature of linkerd, it was just my assumption, that some developers may use it; my only concern was breaking a feature some potentially may be using.
I do not have any incentive to push for this feature, that's why I personally would be fine with maxRequestKB and maxResponseKB gone.

Sorry for confusion.

@adleong

If I understand correctly, the only time the maxRequestKB parameter has any effect is for non-chunked requests with a Content-Length which is larger than maxRequestKB but smaller than streamAfterContentLengthKB. Is that correct?

Not exactly... (ammended my code comments to reflect things better)This is where it gets merky. So if there is Content-Length set and it is larger than maxRequestKB (or than 2GB which is the default set in Finagle), the request will not be processed... To me this behavior is not correct, as it means that you can not have in practice a request with known content length that is larger than 2GB, simply because this is hardecoded in finagle. This is the bit we might want changed in finagle. We can drop our maxRequestKB but until finagle changes to always stream after the fixedLengthStreamedAfter we cannot process fixed size messages larger than 2GB.

My suggestion is do get rid of maxRequestKB and maxResponseKB and rely on finagle's hard bounds and fixedLengthStreamedAfter and submit an issue to finagle that will allow for fixed sized messages to ALWAYS be streamed after the threshold of fixedLengthStreamedAfter. I think this is the best course of action. Alternatively we can leave things as they are. At least now we know what is what. Nevertheless it is worth submitting an issue to finagle just to get more clarity and to verify our understanding (will do that).

Sounds good to me!

@adleong Added a commit to proceed as discussed. Also submitted this issue to finagle to clarify things further

Would you mind updating the PR title and description to match the changes?