mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 67154: Replaced `RpcResult<Response>` with `Try<Response, StatusError>`.
Date Wed, 16 May 2018 21:06:52 GMT


> On May 16, 2018, 8:36 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/grpc.hpp
> > Line 86 (original), 84-93 (patched)
> > <https://reviews.apache.org/r/67154/diff/1/?file=2024140#file2024140line86>
> >
> >     Is this any different than
> >     
> >         StatusError(const ::grpc::Status& _status)
> >           : Error(_status.error_message()),
> >             status(_status) ...
> >             
> >     We currently don't seem to require this to be able to take generic types.

This is for perfect forwarding (i.e., copying when passing const ref and moving when passing
rvalue ref). If you think this is not needed then I'll take your suggestion.


> On May 16, 2018, 8:36 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/grpc.hpp
> > Line 122 (original), 135 (patched)
> > <https://reviews.apache.org/r/67154/diff/1/?file=2024140#file2024140line138>
> >
> >     This would look like a good use case for a hypothetical `Future` with a custom
error type (instead of `std::string` aka `Failure`)
> >     
> >         template <typename T, typename E> struct Future;
> >         
> >     I believe there is a bigger interest in customizing the error type of `Try`
recently, and it might make sense to think about adding that same power to `Future`. I created
https://issues.apache.org/jira/browse/MESOS-8925 to track such work.

This would be a huge work, as it would be very hard to make typed failure to flow through
a chain of futures.


- Chun-Hung


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67154/#review203273
-----------------------------------------------------------


On May 16, 2018, 7:20 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67154/
> -----------------------------------------------------------
> 
> (Updated May 16, 2018, 7:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
> 
> 
> Bugs: MESOS-8924
>     https://issues.apache.org/jira/browse/MESOS-8924
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `process::grpc::client::Runtime::call` method currently returns a
> `RpcResult<Response>`, which contains both a `::grpc::Status` object
> and the resulting response protobuf. However, if the `::grpc::Status`
> represents a non-OK status, the gRPC library does not guarantee that
> the response protobuf is valid. This patch replaces `RpcResult` with
> `Try` to provide better type safety.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436

>   3rdparty/libprocess/src/tests/grpc_tests.cpp 38cd6c61b54518a1019bb11a3551be13026c3f0d

> 
> 
> Diff: https://reviews.apache.org/r/67154/diff/1/
> 
> 
> Testing
> -------
> 
> make check in libprocess
> 
> NOTE: Mesos cannot be built with this patch standalone. The tests are done later in the
chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message