-
Notifications
You must be signed in to change notification settings - Fork 99
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
RSocket with Flowable. #356
RSocket with Flowable. #356
Conversation
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.
I like the change. I didn't find any issues. It looks you will have to rebase anyways so you can address the nits...
LOG(INFO) << "JsonRequestHandler.handleRequestStream " << request; | ||
|
||
// string from payload data | ||
auto pds = request.moveDataToString(); | ||
auto requestString = std::string(pds, request.data->length()); | ||
const char* p = reinterpret_cast<const char*>(request.data->data()); |
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.
nit: I guess you got tired of writing this code so you eventually created Payload::cloneDataToString method.
Do you want to use it over here?
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.
This (and all other callers) can just use Payload::moveDataToString()
as they consume the 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.
I don't understand the question in light of this portion of code.
I created cloneDataToString() because I couldn't get it to work in places where const was required, so I needed a method that copied instead of moved from a const Payload.
The code in question is in ExampleSubscriber:
void ExampleSubscriber::onNext(const Payload& element) noexcept {
That expects a const ref, and because of that I can't move data from it.
LOG(INFO) << "TextRequestHandler.handleRequestStream " << request; | ||
|
||
// string from payload data | ||
auto pds = request.moveDataToString(); | ||
auto requestString = std::string(pds, request.data->length()); | ||
const char* p = reinterpret_cast<const char*>(request.data->data()); |
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.
same question here
LOG(INFO) << "HelloStreamRequestHandler.handleRequestStream " << request; | ||
|
||
// string from payload data | ||
const char* p = reinterpret_cast<const char*>(request.data->data()); |
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.
ditto
yarpl::Reference<yarpl::Flowable<reactivesocket::Payload>> | ||
HelloStreamRequestHandler::handleRequestStream( | ||
reactivesocket::Payload request, | ||
reactivesocket::StreamId streamId) { | ||
LOG(INFO) << "HelloStreamRequestHandler.handleRequestStream " << request; | ||
|
||
// string from payload data | ||
const char* p = reinterpret_cast<const char*>(request.data->data()); |
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.
and here
* nature of RSocket, this can be used on the client as well. | ||
*/ | ||
class RSocketRequestHandler { | ||
public: |
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.
I am happy to see how this interface doesn't have to do anything with setup/resume initialization.
@@ -43,6 +43,13 @@ std::string Payload::moveDataToString() { | |||
return data->moveToFbString().toStdString(); | |||
} | |||
|
|||
std::string Payload::cloneDataToString() const { |
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.
implementing the <<
overload is a bit more idiomatic (and yields nicer code). Like here: https://github.com/ReactiveSocket/reactivesocket-cpp/blob/f1b862a23bc951bbe5869d03321eafa4f583a2a1/src/Common.h#L32
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.
I was matching the moveDataToString
signature. Seems confusing to have one as a name with "move" and another with an operator overload.
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.
By the way, I did not mean we should not have the <<
overload. That would be great to have.
It is somewhat confusing though since this type has both data and metadata. So it seems it would have to work for only one of them, or concat them.
server
client
the logs from server processing the client request:
|
Example code showing how class HelloStreamRequestHandler : public rsocket::RSocketRequestHandler {
public:
/// Handles a new inbound Stream requested by the other end.
yarpl::Reference<yarpl::Flowable<reactivesocket::Payload>>
handleRequestStream(
reactivesocket::Payload request,
reactivesocket::StreamId streamId) override {
LOG(INFO) << "HelloStreamRequestHandler.handleRequestStream " << request;
// string from payload data
auto requestString = request.moveDataToString();
return Flowables::range(1, 10)->map([name = std::move(requestString)](
int64_t v) {
std::stringstream ss;
ss << "Hello " << name << " " << v << "!";
std::string s = ss.str();
return Payload(s, "metadata");
});
}
}; |
@lehecka has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Update the RSocket APIs to have RSocketRequester and RSocketRequestHandler using Flowable.