Skip to content
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

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

Open
aricart opened this issue Jun 3, 2022 · 14 comments
Labels
defect Suspected defect such as a bug or regression

Comments

@aricart
Copy link
Member

aricart commented Jun 3, 2022

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
Copy link
Member Author

aricart commented Jun 3, 2022

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
Copy link
Member Author

aricart commented Jun 3, 2022

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
Copy link
Member

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
Copy link
Member Author

aricart commented Jun 6, 2022

@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
Copy link
Member

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
Copy link
Member Author

aricart commented Jun 6, 2022

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
Copy link
Member

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
Copy link
Member

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

@aricart
Copy link
Member Author

aricart commented Jun 10, 2022

I would love that solution.

@derekcollison
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

Jarema commented Jun 14, 2022

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
Copy link
Contributor

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
Copy link
Member

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.

@bruth bruth added defect Suspected defect such as a bug or regression and removed 🐞 bug labels Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect Suspected defect such as a bug or regression
Projects
None yet
Development

No branches or pull requests

6 participants