Copilot commented on code in PR #3755:
URL: https://github.com/apache/celeborn/pull/3755#discussion_r3548547273
##########
cpp/celeborn/network/Message.cpp:
##########
@@ -218,5 +220,11 @@ void PushMergedData::internalEncodeTo(
protocol::encode(buffer, partitionUniqueIds_);
protocol::encode(buffer, batchOffsets_);
}
+
+std::unique_ptr<Heartbeat> Heartbeat::decodeFrom(
+ std::unique_ptr<memory::ReadOnlyByteBuffer>&& data) {
+ data->skip(1);
+ return std::make_unique<Heartbeat>();
+}
Review Comment:
Heartbeat::decodeFrom unconditionally skips 1 byte and returns a Heartbeat
without validating the frame shape. If a malformed/forward-compatible frame
includes extra bytes (e.g., non-zero bodyLength), this will silently ignore
trailing bytes instead of failing fast.
##########
cpp/celeborn/network/Message.h:
##########
@@ -322,5 +322,14 @@ class PushMergedData : public Message {
std::vector<std::string> partitionUniqueIds_;
std::vector<int32_t> batchOffsets_;
};
+
+class Heartbeat : public Message {
+ public:
+ Heartbeat()
+ : Message(HEARTBEAT, memory::ReadOnlyByteBuffer::createEmptyBuffer()) {}
+
+ static std::unique_ptr<Heartbeat> decodeFrom(
+ std::unique_ptr<memory::ReadOnlyByteBuffer>&& data);
+};
Review Comment:
Heartbeat derives from Message but does not override
internalEncodedLength()/internalEncodeTo(). If a Heartbeat instance is ever
encoded (e.g., future client-side heartbeats), Message::encode will hit the
CELEBORN_UNREACHABLE default and crash. Implementing the minimal encoder keeps
the type symmetric with the Java protocol implementation (1 byte payload set to
0).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
