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

feat: Add option to configure NATS max_payload in JetStream eventbus #2164

Merged
merged 2 commits into from
Sep 4, 2022

Conversation

daniel-codefresh
Copy link
Member

Solves #2163

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>
Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>
@whynowy whynowy merged commit d15693f into argoproj:master Sep 4, 2022
@whynowy
Copy link
Member

whynowy commented Sep 4, 2022

@daniel-codefresh - Sorry, I might have clicked it too fast, max_payload should not be exposed with this approach, instead, it should be done at https://github.com/argoproj/argo-events/blob/master/eventbus/jetstream/base/jetstream.go#L131.

Could you make the change?

whynowy added a commit to whynowy/argo-events that referenced this pull request Sep 4, 2022
…ventbus (argoproj#2164)"

This reverts commit d15693f.

Should use a different approach.

Signed-off-by: Derek Wang <whynowy@gmail.com>
@daniel-codefresh
Copy link
Member Author

daniel-codefresh commented Sep 5, 2022

@daniel-codefresh - Sorry, I might have clicked it too fast, max_payload should not be exposed with this approach, instead, it should be done at https://github.com/argoproj/argo-events/blob/master/eventbus/jetstream/base/jetstream.go#L131.

Could you make the change?

@whynowy not sure I follow - do you mean that max_payload should be passed as part of streamConfig ? if so then I'm not sure it can be done because max_payload should be part of the nats server config and not part of the stream config..
Of course we can create a new serverConfig field to allow users to pass a yaml similarly to how its done with the stream config or with the jetstream settings if thats what you mean (nats.conf):
Screen Shot 2022-09-05 at 13 07 38

@whynowy
Copy link
Member

whynowy commented Sep 6, 2022

A final control is the Maximum Size any single message may have. NATS have it's own limit for maximum size (1 MiB by default), but you can say a Stream will only accept messages up to 1024 bytes using MaxMsgSize.

https://docs.nats.io/using-nats/developer/develop_jetstream/model_deep_dive

Max message size can be configured when creating a stream, see the source code below.

https://github.com/nats-io/nats.go/blob/main/jsm.go#L104

@daniel-codefresh
Copy link
Member Author

A final control is the Maximum Size any single message may have. NATS have it's own limit for maximum size (1 MiB by default), but you can say a Stream will only accept messages up to 1024 bytes using MaxMsgSize.

https://docs.nats.io/using-nats/developer/develop_jetstream/model_deep_dive

Max message size can be configured when creating a stream, see the source code below.

https://github.com/nats-io/nats.go/blob/main/jsm.go#L104

@whynowy maxMsgSize in stream config is respected only if it's smaller than max_payload in NATS config, otherwise max_payload has an higher priority. For instance, if you define a maxMsgSize of 2MB in the stream config and you use the default value of max_payload which is 1MB, then every message above 1MB would still get rejected.

whynowy pushed a commit that referenced this pull request Sep 10, 2022
…2164)

* feat: add option to configure nats max_payload in jetstream eb

Signed-off-by: Daniel Soifer <daniel.soifer@codefresh.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants