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

Pull config changes in ConsumerConfig and JSApiConsumerGetNextRequest #112

Open
9 of 15 tasks
scottf opened this issue May 20, 2022 · 12 comments
Open
9 of 15 tasks

Pull config changes in ConsumerConfig and JSApiConsumerGetNextRequest #112

scottf opened this issue May 20, 2022 · 12 comments
Assignees
Labels
client Client related work enhancement New feature or request Related to JS Simp

Comments

@scottf
Copy link
Collaborator

scottf commented May 20, 2022

Overview

Support for MaxBytes for pull requests including changes to Consumer Configuration and JSApiConsumerGetNextRequest

See Server PR 3126

The Consumer Configuration now includes max_expires and max_bytes

MaxRequestExpires  time.Duration `json:"max_expires,omitempty"`
MaxRequestMaxBytes int `json:"max_bytes,omitempty"`

The JSApiConsumerGetNextRequest now includes max_bytes and idle_heartbeat

MaxBytes  int           `json:"max_bytes,omitempty"`
Heartbeat time.Duration `json:"idle_heartbeat,omitempty"`

The max_bytes limit makes the server complete a batch request if the total number of bytes sent to a pull request reaches this value. It works in combination with batch count, which means that the pull request will be complete based on whichever completes first (count or size).

The idle_hearbeat causes the server to send messages with "100 Idle Heartbeat" status (note that the value cannot be 50% more than the consumer config heartbeat value, but the check is done by the server, library does not have to check for that).

Note max_bytes is currently EXPERIMENTAL while behavior is ironed out.

Clients and Tools

Other Tasks

  • docs.nats.io updated
  • Update ADR to Implemented
  • Update client features spreadsheet

Client authors please update with your progress. If you open issues in your own repositories as a result of this request, please link them to this one by pasting the issue URL in a comment or main issue description.

@scottf scottf added enhancement New feature or request client Client related work labels May 20, 2022
@scottf scottf changed the title Support for MaxBytes for pull requests Support for MaxBytes/Heartbeats for pull requests May 21, 2022
@scottf scottf changed the title Support for MaxBytes/Heartbeats for pull requests Pull config changes in ConsumerConfig and JSApiConsumerGetNextRequest May 21, 2022
@kozlovic
Copy link
Member

Note that we may not rush that to be added in client libraries until the JS simplification work is done and some discussion occurs between client and server team.
For instance, currently the server does not send an indication that the pull request is done because of the size, so the library would need to do accounting of message size + headers when deciding if it needs to wait for more messages or not.
Also, in the case where the pull request max_bytes is smaller than the first message that the server would need to deliver, the server sends a 408 with a description "Message Size Exceeds MaxBytes", which would force libraries to behave differently with this 408 than others..

@scottf
Copy link
Collaborator Author

scottf commented May 25, 2022

I've marked my additions to pull requests as EXPERIMENTAL so we can change them without a break if we need to.

@aricart
Copy link
Member

aricart commented Jun 3, 2022

There are other issues - if the stream has a message that is larger than the max_bytes the consumer will stall, because the server will send a 408, in this case the 408 is final, as there's no way for the message in question to be processed by the client.

nats-io/nats-server#3165

@derekcollison
Copy link
Member

Which max_bytes are you explicitly referring to here?

@scottf
Copy link
Collaborator Author

scottf commented Jun 3, 2022

The new max_bytes on a pull (JSApiConsumerGetNextRequest) Let's say you set max_bytes to 1k, but have a message that is 2k. What will happen?

@derekcollison
Copy link
Member

derekcollison commented Jun 4, 2022

You get this..

nats.Header{"Status": []string{"408"}, "Description": []string{"Message Size Exceeds MaxBytes"}}

@scottf
Copy link
Collaborator Author

scottf commented Jun 4, 2022

It would be nice if we didn't have to rely on text for status messages. We face the same issue with flow control and heartbeats, we have to recognize the status code (100) and the text.

For this though, at a minimum I wonder if there is a better code than 408. Maybe one of these?

  • 412 Precondition Failed
  • 413 Payload Too Large
  • 416 Range Not Satisfiable

At a maximum, is there any way we can just ditch the http based codes here and use an api [like] error code?

@derekcollison
Copy link
Member

I think that is an excellent idea and happy to make that change. We also of course have a precedence of adding in specific error codes to compliment the status codes, which by design are mimicking HTTP.

@derekcollison
Copy link
Member

I don't think we can ditch status alignment with HTTP at this point.

@aricart
Copy link
Member

aricart commented Jun 6, 2022

the one thing that is not great about the current strat, is that these errors are reported simply as status errors, there's no attribution, the only thing making them jetstream is that there's no payload. Since they are a JetStream response, it would be great if they were js API errors. - or headers indicated that these are jetstream errors.

@derekcollison
Copy link
Member

We could possible add more headers, but not sure if that would buy us much. Sometimes it feels like that just means you need to check multiple headers versus one.

How are you thinking about that would work out in practice and for applications?

@scottf scottf removed their assignment Jun 9, 2022
@Jarema Jarema removed their assignment Aug 30, 2022
@ripienaar ripienaar removed their assignment Sep 2, 2022
@bruth
Copy link
Member

bruth commented Sep 8, 2022

Implemented in Go nats-io/nats.go#1043

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Client related work enhancement New feature or request Related to JS Simp
Projects
None yet
Development

No branches or pull requests