[MINOR] fix(client): Fix allocate size by add 9 bytes #1940
Conversation
Test Results 2 791 files - 1 2 791 suites - 1 5h 50m 7s ⏱️ - 1m 35s For more details on these errors, see this check. Results for commit fa8fc8f. ± Comparison against base commit 9fd8ae0. ♻️ This comment has been updated with latest results. |
maobaolong
force-pushed
the
preAllocatedSize
branch
from
ed0c1fa to
5da4167
Compare
| // headerEncodedLength = messageEncodedLength + messageTypeEncodedLength + bodyLength + | ||
| // messageEncodedLength | ||
| // {@link org.apache.uniffle.common.netty.MessageEncoder#encode} | ||
| int headerEncodedLength = |
There was a problem hiding this comment.
I think we need a static variable to represent the 9 bytes of the header.
|
You need to be more careful when refactoring this, or else Uniffle Server might encounter OOM issues under high concurrency.
I think this is OK. Because the preAllocatedSize will be released very soon. |
maobaolong
force-pushed
the
preAllocatedSize
branch
from
95f5364 to
9e36fcc
Compare
| public int getSize() { | ||
| return length + 3 * 8 + 2 * 4; | ||
| // FIXME(maobaolong): The size is calculated based on the Protobuf message ShuffleBlock. | ||
| // If Netty's custom serialization is used, the calculation logic here needs to be modified. |
There was a problem hiding this comment.
Do you mean netty still use the protobuf de/serialization?
There was a problem hiding this comment.
In fact, Netty need only data size is enough, but for now server release usedmemory contains the block encoded size
There was a problem hiding this comment.
In fact, Netty need only data size is enough
If so, why not removing the unused placeholder bits directly? Just for unified logic for grpc and netty?
There was a problem hiding this comment.
+1 for this after reviewing this netty based network protocol. Although I think we should bind the unique requireBufferId with the required buffer size that is determinzed by the client side, that means we could release the allocated size more precise in the next accepting data in the server side logic
|
Please fix the conflict. After that, I think I can merge this assp. @maobaolong |
maobaolong
force-pushed
the
preAllocatedSize
branch
from
9e36fcc to
4f97b2d
Compare
|
@zuston Conflicts has been resolved, but after review this PR, I found it hard to keep consistent with previous version, and the client and the server should be updated together. |
maobaolong
force-pushed
the
preAllocatedSize
branch
from
4f97b2d to
fa8fc8f
Compare
What changes were proposed in this pull request?
Improve the preAllocated buffer size.
Why are the changes needed?
(Please clarify why the changes are needed. For instance,
Without this change, this encoded length always less 9 byte than byteBuf#readableSize after encoded.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
No need.