[CELEBORN-2377] Support Heartbeat message decoding in C++ client#3755
[CELEBORN-2377] Support Heartbeat message decoding in C++ client#3755Kalvin2077 wants to merge 2 commits into
Conversation
Add Heartbeat message type with decodeFrom support in Message.h/Message.cpp and a decodeHeartbeat unit test in MessageTest.cpp.
There was a problem hiding this comment.
Pull request overview
Adds decoding support for the HEARTBEAT (type id 22) transport message in the C++ client so inbound heartbeat frames no longer fail in Message::decodeFrom, and includes a unit test covering the new decode path.
Changes:
- Add
HEARTBEAThandling toMessage::decodeFromand introduce a C++Heartbeatmessage type with a decoder. - Add a new gtest
MessageTest.decodeHeartbeatthat constructs a heartbeat wire frame and validates decoding. - Extend the C++ message type switch to route heartbeat frames to the new decoder.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cpp/celeborn/network/Message.cpp | Routes HEARTBEAT through the decoder and adds Heartbeat::decodeFrom. |
| cpp/celeborn/network/Message.h | Introduces the Heartbeat message class in the C++ protocol layer. |
| cpp/celeborn/network/tests/MessageTest.cpp | Adds a unit test verifying heartbeat wire-frame decoding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
@Kalvin2077, thanks for contribution. I have aligned the heartbeat decode path end-to-end against the Java wire format (Heartbeat.encodedLength()==1, encode writes one byte) — the header parsing, the CELEBORN_CHECK_EQ(encodedLength + bodyLength, remainingSize()) guard, and Heartbeat::decodeFrom's skip(1) all line up exactly with the wire, so the decode itself is correct and this rightly replaces the previous CELEBORN_FAIL that would tear down the connection. Left a few notes below; the dispatcher one is the main item, the other two are minor/optional. Please also evaluate and address review comments of Copilot.
…rt, test coverage - Add HEARTBEAT case to MessageDispatcher::read so heartbeats are consumed silently instead of hitting the ERROR default branch. - Add internalEncodedLength/internalEncodeTo and copy constructor to Heartbeat, matching sibling Message subclasses and Java wire format. - Add encodeHeartbeat test verifying the encoded frame. - Add heartbeatIsSilentlyConsumed dispatcher test verifying heartbeat does not affect pending requests or close the connection. - Add body()->remainingSize()==0 assertion to decodeHeartbeat test.
|
Thanks. Merged to main(v0.7.0). |
What changes were proposed in this pull request?
This PR adds Heartbeat message decoding support to the C++ client network.
Why are the changes needed?
The HEARTBEAT type (id 22) is already declared in the Message::Type enum and recognized by decodeType, but Message::decodeFrom had no case for it, so any heartbeat frame received by the C++ client hit the unsupported Message decode type failure path. As the protocol/server side begins to send heartbeat messages, the C++ client must be able to decode them instead of throwing.
Does this PR resolve a correctness bug?
Does this PR introduce any user-facing change?
How was this patch tested?
Added MessageTest.decodeHeartbeat, which constructs a heartbeat wire frame and asserts that Message::decodeFrom returns a non-null message with type HEARTBEAT. Built and ran the test with -- gtest_filter="MessageTest.decodeHeartbeat" inside the jraaaay/celeborn-cpp-dev:0.4 container; it passed.