FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[PIP-132] Include message header size when check maxMessageSize for non-batch message on the client side. by Jason918 · Pull Request #14007 · apache/pulsar · GitHub

/ pulsar Public

[PIP-132] Include message header size when check maxMessageSize for non-batch message on the client side. - #14007

Merged
codelipenghui merged 6 commits into
apache:masterfrom
Jason918:PIP-132
Mar 8, 2022
Merged

codelipenghui merged 6 commits into
apache:masterfrom
Jason918:PIP-132

Conversation

Jason918 commented Jan 28, 2022
edited
Loading

Copy link
Copy Markdown
Contributor

Master Issue: #13591

Motivation

See #13591

Modifications

  1. Add max message size check before sending request for non-batch and non-chunked messages.
  2. Decrease chunk size by metadata size for chunked messages.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • pulsar-broker/src/test/java/org/apache/pulsar/broker/service/MaxMessageSizeTest.java

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • no-need-doc

Jason918 requested review from eolivelli, merlimat and michaeljmarshall and removed request for eolivelli and michaeljmarshall January 28, 2022 11:53
github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jan 28, 2022
Jason918 closed this Jan 29, 2022
Jason918 reopened this Jan 29, 2022

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

codelipenghui added this to the 2.11.0 milestone Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

Copy link
Copy Markdown
Contributor Author

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

Jason918 requested a review from aloyszhang February 27, 2022 02:54

aloyszhang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Jason918 commented Mar 8, 2022

Copy link
Copy Markdown
Contributor Author

@RobertIndie PTAL

RobertIndie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

codelipenghui merged commit 791853f into apache:master Mar 8, 2022
gaozhangmin pushed a commit to gaozhangmin/pulsar that referenced this pull request Mar 8, 2022
…on-batch message on the client side. (apache#14007)

Master Issue: apache#13591

### Motivation

See apache#13591

### Modifications

1. Add max message size check before sending request for non-batch and non-chunked messages.
2. Decrease chunk size by metadata size  for chunked messages.
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
…on-batch message on the client side. (apache#14007)

Master Issue: apache#13591

### Motivation

See apache#13591

### Modifications

1. Add max message size check before sending request for non-batch and non-chunked messages.
2. Decrease chunk size by metadata size  for chunked messages.
BewareMyPower added a commit to BewareMyPower/pulsar that referenced this pull request Jun 23, 2022
…hunks after PIP-132

Fixes apache#16195

### Motivation

[PIP-132](apache#14007) considers the
message metadata size when computing the payload chunk size and the
number of chunks. However, it could make some messages whose size is
less than `maxMessageSize` cannot be sent. There are two reasons:
1. The `MessageMetadata` will be updated after computing the payload
   chunk size, i.e. the actual metadata size would be greater.
2. `OpSendMsg#getMessageHeaderAndPayloadSize` doesn't exclude all bytes
   other than the metadata and payload, e.g. the 4 bytes checksum field.

For example, if the max message size is 100, send a string whose size is
60 with chunking enabled.
1. The initial metadata size is 25 so the chunk size is 75, the message
   won't be spit into chunks.
2. After `serializeAndSendMessage`, the metadata size becomes 32, so the
   serialized header's total size is 4 + 8 + 6 + 4 + 32 = 54, and the
  total size is 54 + 60 = 114, see `headerContentSize` in
  `serializeCommandSendWithSize`.
3. In `getMessageHeaderAndPayloadSize`, the returned value is computed
   by 114 - 8 - 4 = 102 > 100. The 6 bytes magic and checksum and 4
   bytes metadata length field are not included.

### Modifications

- Update the message metadata before computing the chunk size.
- Compute the correct size in `getMessageHeaderAndPayloadSize`.

### Verifying this change

Add `testChunkSize` to verify all sizes in range [1, maxMessageSize] can
be sent successfully when chunking is enabled.
BewareMyPower added a commit that referenced this pull request Jun 28, 2022
…hunks after PIP-132 (#16196)

Fixes #16195

### Motivation

[PIP-132](#14007) considers the
message metadata size when computing the payload chunk size and the
number of chunks. However, it could make some messages whose size is
less than `maxMessageSize` cannot be sent. There are two reasons:
1. The `MessageMetadata` will be updated after computing the payload
   chunk size, i.e. the actual metadata size would be greater.
2. `OpSendMsg#getMessageHeaderAndPayloadSize` doesn't exclude all bytes
   other than the metadata and payload, e.g. the 4 bytes checksum field.

For example, if the max message size is 100, send a string whose size is
60 with chunking enabled.
1. The initial metadata size is 25 so the chunk size is 75, the message
   won't be spit into chunks.
2. After `serializeAndSendMessage`, the metadata size becomes 32, so the
   serialized header's total size is 4 + 8 + 6 + 4 + 32 = 54, and the
  total size is 54 + 60 = 114, see `headerContentSize` in
  `serializeCommandSendWithSize`.
3. In `getMessageHeaderAndPayloadSize`, the returned value is computed
   by 114 - 8 - 4 = 102 > 100. The 6 bytes magic and checksum and 4
   bytes metadata length field are not included.

### Modifications

- Update the message metadata before computing the chunk size.
- Compute the correct size in `getMessageHeaderAndPayloadSize`.

### Verifying this change

Add `testChunkSize` to verify all sizes in range [1, maxMessageSize] can
be sent successfully when chunking is enabled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs type/PIP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL