-
Notifications
You must be signed in to change notification settings - Fork 374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CELEBORN-1814][CIP-14] Add transportMessage to cppClient #3042
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
#include "celeborn/protocol/TransportMessage.h" | ||
#include "celeborn/utils/Exceptions.h" | ||
|
||
namespace celeborn { | ||
TransportMessage::TransportMessage(MessageType type, std::string&& payload) | ||
: type_(type), payload_(std::move(payload)) { | ||
messageTypeValue_ = type; | ||
} | ||
|
||
TransportMessage::TransportMessage(std::unique_ptr<ReadOnlyByteBuffer> buf) { | ||
int messageTypeValue = buf->read<int32_t>(); | ||
int payloadLen = buf->read<int32_t>(); | ||
CELEBORN_CHECK_EQ(buf->remainingSize(), payloadLen); | ||
CELEBORN_CHECK(MessageType_IsValid(messageTypeValue)); | ||
type_ = static_cast<MessageType>(messageTypeValue); | ||
messageTypeValue_ = type_; | ||
payload_ = buf->readToString(payloadLen); | ||
} | ||
|
||
std::unique_ptr<ReadOnlyByteBuffer> TransportMessage::toReadOnlyByteBuffer() | ||
const { | ||
int bufSize = payload_.size() + 4 + 4; | ||
auto buffer = ByteBuffer::createWriteOnly(bufSize); | ||
buffer->write<int>(messageTypeValue_); | ||
buffer->write<int>(payload_.size()); | ||
buffer->writeFromString(payload_); | ||
CELEBORN_CHECK_EQ(buffer->size(), bufSize); | ||
return ByteBuffer::toReadOnly(std::move(buffer)); | ||
} | ||
} // namespace celeborn |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
#pragma once | ||
|
||
#include <folly/io/Cursor.h> | ||
#include <string> | ||
|
||
#include "celeborn/memory/ByteBuffer.h" | ||
#include "celeborn/proto/TransportMessagesCpp.pb.h" | ||
|
||
namespace celeborn { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we consider use nested namespace? eg: namespace celeborn::proto::TransportMessage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think nested namespace is not that necessary. The nested namespace is to avoid naming conflict within the celebornCpp project (as the celebornCpp project is already protected in namespace celeborn and won't conflict with other projects), which is unlikely to happen as the celebornCpp is only for client side and is not for too much complexity. In contrast, a flat celeborn namespace would make it easier to use. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Personally I prefer nested namespaces. I believe that using nested namespaces can enhance the clarity and extensibility of our code for future development. Additionally, I've noticed that namespace::core is utilized in BaseConf.cpp, which used this approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The nested namespace will be better for future extensions of this native part. We wanted to utilize the native capabilities for some parts of the worker. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nested namespace added to TransportMessage. |
||
class TransportMessage { | ||
public: | ||
TransportMessage(MessageType type, std::string&& payload); | ||
|
||
TransportMessage(std::unique_ptr<ReadOnlyByteBuffer> buf); | ||
|
||
std::unique_ptr<ReadOnlyByteBuffer> toReadOnlyByteBuffer() const; | ||
|
||
MessageType type() const { | ||
return type_; | ||
} | ||
|
||
std::string payload() const { | ||
return payload_; | ||
} | ||
|
||
private: | ||
MessageType type_; | ||
int messageTypeValue_; | ||
std::string payload_; | ||
}; | ||
} // namespace celeborn |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
#include <gtest/gtest.h> | ||
|
||
#include "celeborn/proto/TransportMessagesCpp.pb.h" | ||
#include "celeborn/protocol/TransportMessage.h" | ||
|
||
using namespace celeborn; | ||
|
||
TEST(TransportMessageTest, constructFromPayload) { | ||
const std::string payload = "payload"; | ||
auto payloadToMove = payload; | ||
auto messageType = 10; | ||
auto transportMessage = std::make_unique<TransportMessage>( | ||
static_cast<MessageType>(messageType), std::move(payloadToMove)); | ||
|
||
auto transportMessageBuffer = transportMessage->toReadOnlyByteBuffer(); | ||
EXPECT_EQ( | ||
transportMessageBuffer->remainingSize(), | ||
sizeof(int) * 2 + payload.size()); | ||
EXPECT_EQ(transportMessageBuffer->read<int>(), messageType); | ||
EXPECT_EQ(transportMessageBuffer->read<int>(), payload.size()); | ||
EXPECT_EQ(transportMessageBuffer->readToString(), payload); | ||
} | ||
|
||
TEST(TransportMessageTest, constructFromReadOnlyBuffer) { | ||
const std::string payload = "payload"; | ||
auto payloadToMove = payload; | ||
auto messageType = 10; | ||
auto contentMessage = std::make_unique<TransportMessage>( | ||
static_cast<MessageType>(messageType), std::move(payloadToMove)); | ||
auto contentBuffer = contentMessage->toReadOnlyByteBuffer(); | ||
|
||
auto transportMessage = | ||
std::make_unique<TransportMessage>(std::move(contentBuffer)); | ||
auto transportMessageBuffer = transportMessage->toReadOnlyByteBuffer(); | ||
EXPECT_EQ( | ||
transportMessageBuffer->remainingSize(), | ||
sizeof(int) * 2 + payload.size()); | ||
EXPECT_EQ(transportMessageBuffer->read<int>(), messageType); | ||
EXPECT_EQ(transportMessageBuffer->read<int>(), payload.size()); | ||
EXPECT_EQ(transportMessageBuffer->readToString(), payload); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although the default behavior for
addlibrary
is STATIC, this part is inconsistent with other cmake files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that only the proto module explicitly declares STATIC, and other modules don't declare it. So this is consistent to other modules. Yet adding the STATIC declaration brings no harm, so this cmakelist is refactored as requested.
We could refactor the makelists in the following commit to add the STATIC to all makelists.