Skip to content

Conversation

ni-ze
Copy link
Contributor

@ni-ze ni-ze commented May 18, 2022

Make consistent semantics of maxMessageSize, refers to the message body size.

In current version, maxMessageSize refers to message total length not only include message body but also include others fields, such as magic code, queueId, body crc, properties etc.

If we make length limit base on message body, we can get consistent semantics of maxMessageSize easily.

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2022

Codecov Report

Merging #4338 (085ddee) into develop (84fb5e1) will decrease coverage by 0.05%.
The diff coverage is 81.91%.

@@              Coverage Diff              @@
##             develop    #4338      +/-   ##
=============================================
- Coverage      48.20%   48.15%   -0.06%     
+ Complexity      5087     5074      -13     
=============================================
  Files            642      642              
  Lines          42762    42734      -28     
  Branches        5596     5590       -6     
=============================================
- Hits           20614    20578      -36     
- Misses         19637    19647      +10     
+ Partials        2511     2509       -2     
Impacted Files Coverage Δ
...cketmq/broker/processor/ReplyMessageProcessor.java 55.11% <0.00%> (ø)
...he/rocketmq/client/producer/DefaultMQProducer.java 57.34% <ø> (ø)
...org/apache/rocketmq/store/DefaultMessageStore.java 55.56% <0.00%> (ø)
...pache/rocketmq/store/dledger/DLedgerCommitLog.java 76.43% <37.50%> (+0.36%) ⬆️
...etmq/broker/processor/EndTransactionProcessor.java 51.40% <50.00%> (ø)
...ache/rocketmq/store/config/MessageStoreConfig.java 61.72% <66.66%> (ø)
...main/java/org/apache/rocketmq/store/CommitLog.java 75.23% <90.90%> (+0.14%) ⬆️
...ocketmq/broker/processor/SendMessageProcessor.java 39.79% <100.00%> (ø)
...rg/apache/rocketmq/common/stats/StatsSnapshot.java 84.61% <0.00%> (-15.39%) ⬇️
.../apache/rocketmq/common/stats/MomentStatsItem.java 38.09% <0.00%> (-9.53%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84fb5e1...085ddee. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 52.129% when pulling 085ddee on ni-ze:messageLength-2 into 51cf962 on apache:develop.

@coveralls
Copy link

coveralls commented May 18, 2022

Coverage Status

Coverage increased (+0.07%) to 52.047% when pulling 01e7d85 on ni-ze:messageLength-2 into 51cf962 on apache:develop.

@ni-ze ni-ze closed this May 24, 2022
@ni-ze ni-ze reopened this May 24, 2022
duhenglucky
duhenglucky previously approved these changes May 27, 2022
@duhenglucky duhenglucky dismissed their stale review May 27, 2022 05:50

There may be other compatibility issues, especially in MessageExtEncoder class

Copy link
Member

@dongeforever dongeforever left a comment

Choose a reason for hiding this comment

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

The code seems OK.

And it is better to add some tests, Sending a message with the body size equal to the maxMessageSize should succeed.

@ni-ze
Copy link
Contributor Author

ni-ze commented Jun 1, 2022

sure, I will add a unit test.

@duhenglucky duhenglucky merged commit 0bc3b99 into apache:develop Jun 1, 2022
@duhenglucky duhenglucky added this to the 4.9.4 milestone Jun 1, 2022
GenerousMan pushed a commit to GenerousMan/rocketmq that referenced this pull request Aug 12, 2022
…ndback (apache#4338)

* fix(bug)msg length exceeds the maximum length when sendback

* fix(store) MessageStoreConfig: maxMessageBodySize -> maxMessageSize, for compatibility

* style(formatting) commigLog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants