[ISSUE#4468] Optimize broker buffer length initialization #4469
Conversation
| //Reserve 64kb for encoding buffer outside body | ||
| int maxMessageSize = maxMessageBodySize + 64 * 1024; | ||
| byteBuf = alloc.directBuffer(maxMessageSize); |
There was a problem hiding this comment.
Hi, I wonder to know why it is 64KB?
There was a problem hiding this comment.
Most of parameters in the message encoding are fixed length, only the properties and tpopic name content are variable. The maximum length of properties should be less than or equal to Short.MAX_VALUE, and the topic name content should be less than or equal to 127 bytes. Taking the maximum value, it adds up to about 33kb. Considering to reserve some space for subsequent adjustments, 64kb was chosen.
- org.apache.rocketmq.client.Validators#TOPIC_MAX_LENGTH
public static final int TOPIC_MAX_LENGTH = 127;- org.apache.rocketmq.store.CommitLog.MessageExtEncoder#encode
if (propertiesLength > Short.MAX_VALUE) {
log.warn("putMessage message properties length too long. length={}", propertiesData.length);
return new PutMessageResult(PutMessageStatus.PROPERTIES_SIZE_EXCEEDED, null);
}Note: The maximum length of the topic is set on the client side, and there is no open interface for modification. Besides, the broker side does not provide the parameter "TOPIC_MAX_LENGTH", so I directly select the extra space as a fixed value.
There was a problem hiding this comment.
Hi,
Thx for your contribution in advance. But we could make a discuss if it is worth making this change.
The ByteBuffer used here was from Netty API, and it already had an implementation of autoscaling under it. Whenever the ByteBuf reaches the initialCapacity set by developers, it would start to autoscale by 4MB until maxCapacity (set by devs) or Integer.MAX_VALUE.
Of course, we do not want autoscaling it every time, but if the chance of exceeding the maxMessageBodySize is not that high, I don't see why we need to add another 64KB (from your changes) for ByteBuf.
Yes, using bytebuf can achieve autoscaling, so in the current scenario, no error will be reported. But what I consider is that mq should have its own detection of the encoding length of the message, instead of relying on the self-autoscaling of bytebuf. As for why it is 64kb, I have replied in a previous comment. |
|
@shengminw Okay, that makes sense. Thx for your reply. Maybe @ni-ze would like to check this out. |
|
@tsunghanjacktsai ok, thanks~ |
| CommitLog.PutMessageThreadLocal putMessageThreadLocal = commitLog.getPutMessageThreadLocal().get(); | ||
| PutMessageResult encodeResult1 = putMessageThreadLocal.getEncoder().encode(messageExtBrokerInner); | ||
| assertTrue(encodeResult1 == null); | ||
|
|
There was a problem hiding this comment.
@shengminw This test is for DefaultMessageStore.
So just use type conversion (DefaultMessageStore) messagestore, instead of reflection.
The reflection is not friendly for further evolution.
There was a problem hiding this comment.
BTW, it maybe need another test for body size exactly equal to max message size.
There was a problem hiding this comment.
ok, I will revise it.
Codecov Report
@@ Coverage Diff @@
## develop #4469 +/- ##
=============================================
- Coverage 48.16% 48.04% -0.12%
+ Complexity 5127 5103 -24
=============================================
Files 649 649
Lines 43021 43026 +5
Branches 5625 5626 +1
=============================================
- Hits 20720 20672 -48
- Misses 19798 19845 +47
- Partials 2503 2509 +6
Continue to review full report at Codecov.
|
| MessageExtEncoder(final int maxMessageBodySize) { | ||
| ByteBufAllocator alloc = UnpooledByteBufAllocator.DEFAULT; | ||
| byteBuf = alloc.directBuffer(maxMessageBodySize); | ||
| //Reserve 64kb for encoding buffer outside body | ||
| int maxMessageSize = maxMessageBodySize + 64 * 1024; |
There was a problem hiding this comment.
IMO, variable parameters, which is properties and topic name, is limited and properties before write into commitLog, if this two item length exceed limit, it won't reach here.
and I think it is a little weird to limit length base on an auto scalable ByteBuf.
There was a problem hiding this comment.
Yes, get to this step, properties(max 32kb) and topic name(max 127 bytes) are limited. But the full message length still may exceed 4M(maxMessageBodySize).
About whether it is neccessary to do this change, it's worth discussing. I think, in the previous version, maxMessageBodySize is also used to limit buffer length. So from my point of view, I think it is necessary to add.
What is the purpose of the change
Optimize encoder buffer initialization, expand buffer length to avoid message size exceeding when messageBodySize is on the boundry of maxMessageBodySize.
Brief changelog
add maxMessageSize variable differing from maxMessageBodySize（default maxMessageSize = maxMessageBodySize + 64kb）
add checking messagesize
add junit test "testEncodeLongMessage()"