nats-io / nats-server

High-Performance server for NATS.io, the cloud and edge native messaging system.
https://nats.io
Apache License 2.0
15.95k stars 1.41k forks source link

`max_bytes` consumer configurations perhaps shouldn't be smaller than it's streams `max_msg_size` #3165

Open aricart opened 2 years ago

aricart commented 2 years ago

Defect

On a pull subscriber, it is now possible to request the maximum number of bytes to be handed on the request. Currently, this feature will reject sending messages if the messages on the stream are larger than the max_bytes. This is reasonable, but it creates a new category of errors perhaps, because 404, 408s, and 409 are arguably not really errors but simply, there are no messages right now type of thing - arguably message exceeded is similar.

The one difference though is that it is possible to create a consumer that will never make progress because messages on the stream are not the right size ever. This means that we might want some relation between the max_msg_size on the stream and the max_bytes- as in max_bytes shouldn't be smaller than max_msg_size. The relation/check between these settings would prevent clients that present values as an iterator etc - where the user is not checking errors because errors are a terminal condition, to fail if this condition is true, and it would also keep producers honest if they exceed those payload sizes.

aricart commented 2 years ago

If the intention is keep my buffer on the small side, perhaps it is acceptable to send a larger message, but this would need to be documented that max_bytes becomes then a 1 message limit possibly. This perhaps would be more useful.

aricart commented 2 years ago

more interesting, the consumer will run aground if a single message in the stream has a larger payload - so currently this is a terminal condition for the consumer.

derekcollison commented 2 years ago

If you ask for 10k let's say and there is 1 msg but it's bigger than that I believe we send back enough information for the client to figure that out. But hopefully most users to not fiddle with this setting at all.

aricart commented 2 years ago

@derekcollison it is a terminal condition because any pull with a lower value will always fail. Possibly this is correct, but since NATS and JetStream have an unit of "message", it seems wrong to not send the one message even if exceeds. IMHO the max_bytes is simply a way for the client to suggest that when more than one message, to not send more messages that the amount of data specified, this shouldn't ever be less than a single message.

derekcollison commented 2 years ago

If they resend request with lower value it will succeed. Maybe we disclose in additional header a hint?

Also I hear you on send at least one. The reason though I decided against it was for small devices. If they only have so much memory this needs to be strict IMO. Especially if they say 256k and someone accidentally sends an 8Mb message for instance.

aricart commented 2 years ago

correct, but then at that point, is the reasoning for this PR that if a client wants to use max_bytes in a pull, the value shouldn't be allowed to be smaller than what the stream allows or the client will be blocked forever (or the contract you specify will be broken anyway on an auto-increase). In cases where the pull consumer is exported to another account (where only next() is exposed), this also becomes a problem, as the exported consumer will not be able to verify the limit.

I think this is an undue complication, the NATS unit of data is the one message, so max_bytes is only some sort of buffering strategy.

derekcollison commented 2 years ago

Could be reasonable for now to say that the largest possible message that could be in a stream should be the smallest value allowed in a pull request with MaxBytes set..

I am not sure I foresee folks using this directly, but this is how I believe we will do emulated push, with options to tweak.

derekcollison commented 2 years ago

I am open to considering that at least one message will be sent as well.

aricart commented 2 years ago

I would love that solution.

derekcollison commented 2 years ago

I have been thinking about this and still not sure that behavior is correct. If you ask for 1 message we don;'t give you 2, and if you ask for zero we don't give you 1. Assume you get bad request or something.

Maybe we add in an additional header to get you unblocked?

Interested in what @matthiashanel and @Jarema think here.

matthiashanel commented 2 years ago

I would go with sending at least one message. In part because at the moment max_msg_size is optional. This also seems less complicated to implement than, say having to make adjustments based on headers and re-sending requests.

If buffer limits are a concern, I'd expect the user to set max_msg_size on streams consumed by these devices to what they need.

Jarema commented 2 years ago

I agree with @matthiashanel

I agree that if you request 1 message, we don't give you two, but that's easy, as a message is a "quantum" in NATS protocol. There is no ambiguity about where batch request ends if you refer to those "quantum". You get the exact number and that's it.

It's different if you talk about bytes. Although NATS protocol does not know about the concept of "part of a message" a max_bytes limit can, and almost always will, kick in somewhere in the middle of a message. If the message is bigger than max_bytes, it will be the first one.

I would define max_bytes as a "threshold", that when passed while processing a message, that message is the last one delivered.

That would allow consistent behavior - no matter if your max_bytes is bigger than the first message, or if the server hits that limit while processing 200th message of the batch.

This in turn is, in my opinion, simpler to implement on the server - no worrying about how to handle that 200th message that server is already processing but hit the threshold.

For a user, it would also be cleaner and less surprising - getting at least one message for a given batch. We could even add some header letting the user know that while processing this message max_bytes was reached and maybe even add a more specific one, which tells that this message alone was bigger than the max_bytes, hence user got only it.

That means that max_bytes is technically max_bytes_threshold. WDYT @derekcollison ?

ripienaar commented 2 years ago

Essentially this is a stalled consumer then and we need to do in general better with those, we've discussed telling users why a consumer is stalled - FC blocked, all outstanding acks busy etc - there should be a clear way to find out is my consumer functionining or just will never recover.

FC and Acks one can perhaps do something about, max bytes would be terminal (unless consumer is edited).

So all these stalls are super surprising to users and if we lean in on making hard stalls like this we need to make the why discoverable at the same time.

derekcollison commented 2 years ago

I agree we need to improve why consumers are stalled. I think this fits better into that category to be honest. You can have the client adjust the next pull request if so desired to allow for a larger amount. And again, this can happen, but I am not sure I see this as a normal use case.

Also in terms of bytes, when we enforce them they are a hard limit, look at slow consumer semantics.