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.

Sign up for GitHub

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

Jump to bottom

Due to internal limits the actual maximum payload size is 64Mb. Beyon… #2407

Merged
merged 1 commit into from Aug 4, 2021
Merged

Due to internal limits the actual maximum payload size is 64Mb. Beyon… #2407

merged 1 commit into from Aug 4, 2021

Conversation

jnmoyne
Copy link
Contributor

@jnmoyne jnmoyne commented Aug 4, 2021

…d that the subscribing process is unable to receive the messages, so adjusting the maximum size the server allows you to set for max_payload.

  • Link to issue, e.g. Resolves #NNN
  • Documentation added (if applicable)
  • Tests added
  • Branch rebased on top of current main (git pull --rebase origin main)
  • Changes squashed to a single commit (described here)
  • Build is green in Travis CI
  • You have certified that the contribution is your original work and that you license the work to the project under the Apache 2 license

Resolves #

Changes proposed in this pull request:

  • There was an existing check on the value specified in the configuration for max_payload, adjusted the max value to be 64MB

/cc @nats-io/core

@jnmoyne
Due to internal limits the actual maximum payload size is 64Mb. Beyon…
b5e3f8d 
…d that the subscribing process is unable to receive the messages, so adjusting the maximum size the server allows you to set for `max_payload`.
derekcollison
derekcollison approved these changes Aug 4, 2021
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thx

@derekcollison derekcollison merged commit 883e287 into nats-io:main Aug 4, 2021
@jnmoyne jnmoyne deleted the fix-max-max-payload-value branch August 4, 2021 15:14
@kozlovic
Copy link
Member

kozlovic commented Aug 4, 2021

@jnmoyne @derekcollison Having second thoughts on this one. What would be the internal limit that would cause the max_payload to be limited to 64MB (note that I personally think that one should not set a max_payload this high anyway, so ok with limiting, but..).
If the limit you mention is the MaxPending (default 64MB), this one is also configurable (see option MaxPending).

@derekcollison
Copy link
Member

It was the default max for the dynamic buffers etc IIRC.

I still like it, agree something off if you are going higher..

kozlovic added a commit that referenced this pull request Aug 4, 2021
@kozlovic
[CHANGED] Check that max_payload is not greater than max_pending
4865dc7 
This is related to PR #2407. Since the 64MB pending size is actually
configurable, we should fail only if max_payload is greater than
the configured max_pending. This is done in validateOptions() which
covers both config file and direct options in embedded cases.
The check in opts.go is reverted to max int32 since at this point
we don't know if/what max_pending will be, so we simply check
that it is not more than a int32.

For the next minor release, we could have another change that
imposes a lower limit to max_payload (regardless if max_pending
is higher).

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic kozlovic mentioned this pull request Aug 4, 2021
Merged
@jnmoyne
Copy link
Contributor Author

jnmoyne commented Aug 4, 2021

I actually checked first if the limit was max_pending rather than specifically 64MB and it's actually not. If you set max_pending (and max_payload) to something more than 64MB and try to send a message above 64MB you get a different error message on the server and slightly different behavior on the subscriber side but it still doesn't work.

@kozlovic
Copy link
Member

kozlovic commented Aug 4, 2021

Works for me:

more c.conf 
max_payload=68157440
max_pending=68157440

server starts:

 nats-server -c c.conf 
[35861] 2021/08/04 17:28:11.370717 [INF] Starting nats-server
[35861] 2021/08/04 17:28:11.370847 [INF]   Version:  2.3.4
[35861] 2021/08/04 17:28:11.370849 [INF]   Git:      [not set]
[35861] 2021/08/04 17:28:11.370854 [INF]   Name:     NAJFTWOGUL64RCJ7WOFFNSYLMWNR7TKFNCVMMMPW2GUGI7555GJ45H37
[35861] 2021/08/04 17:28:11.370856 [INF]   ID:       NAJFTWOGUL64RCJ7WOFFNSYLMWNR7TKFNCVMMMPW2GUGI7555GJ45H37
[35861] 2021/08/04 17:28:11.370858 [INF] Using configuration file: c.conf
[35861] 2021/08/04 17:28:11.370867 [WRN] Maximum payloads over 8.00 MB are generally discouraged and could lead to poor performance
[35861] 2021/08/04 17:28:11.371687 [INF] Listening for client connections on 0.0.0.0:4222
[35861] 2021/08/04 17:28:11.371914 [INF] Server is ready

Then use nats-bench to send a 64MB+1024:

go run examples/nats-bench/main.go -np 1 -ns 1 -n 10 -ms 67109888 foo
Starting benchmark [msgs=10, msgsize=67109888, pubs=1, subs=1]
NATS Pub/Sub stats: 36 msgs/sec ~ 2.28 GB/sec
 Pub stats: 18 msgs/sec ~ 1.15 GB/sec
 Sub stats: 25 msgs/sec ~ 1.60 GB/sec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekcollison derekcollison derekcollison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jnmoyne @kozlovic @derekcollison