Re: Review Request 51493: Add the query parameter in createRequest.

[Joris Van Remoortere]

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



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On Aug. 29, 2016, 10:47 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51493/
> ---
> 
> (Updated Aug. 29, 2016, 10:47 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-4440
> https://issues.apache.org/jira/browse/MESOS-4440
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add the new 'query' parameter in createRequest method, which will be used by 
> Http GET method later.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 404196bb198c1ff958b55d72fb29c5fe92dba429 
>   3rdparty/libprocess/src/http.cpp 298bd460afdeccd18e201d8a505b961fd4cf3d3e 
> 
> 
> Diff: https://reviews.apache.org/r/51493/diff/1/
> 
> 
> Testing
> ---
> 
> make & make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 51493: Add the query parameter in createRequest.

[Adam B]

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



Handle the error, and then we'll consider this an appropriate replacement for 
`http::get()`


3rdparty/libprocess/src/http.cpp (lines 1328 - 1329)


If you're accepting a `Try`, you must handle the error before assuming you 
can `get()`. That's what http::get() was doing before:
```
if (decode.isError()) {
  return Failure("Failed to decode HTTP query string: " + 
decode.error());
```
Handling the error means that this form of `createRequest` would return a 
`Try`, which `http::request()` (and all other call-sites) will have to 
handle the `Error` and bubble up appropriately as a failed Future.


- Adam B


On Aug. 29, 2016, 3:47 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51493/
> ---
> 
> (Updated Aug. 29, 2016, 3:47 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-4440
> https://issues.apache.org/jira/browse/MESOS-4440
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add the new 'query' parameter in createRequest method, which will be used by 
> Http GET method later.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 404196bb198c1ff958b55d72fb29c5fe92dba429 
>   3rdparty/libprocess/src/http.cpp 298bd460afdeccd18e201d8a505b961fd4cf3d3e 
> 
> Diff: https://reviews.apache.org/r/51493/diff/
> 
> 
> Testing
> ---
> 
> make & make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 51493: Add the query parameter in createRequest.

[Yongqiao Wang]

> On 九月 11, 2016, 5:26 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/include/process/http.hpp, line 845
> > 
> >
> > I think use `hashmap` as type here would be better?

Thank haosdent for your comments, to keep the interface consistence with 
`http:get(const UPID& upid,const Option& path, const Option& 
query,const Option& headers)`, I also use `Option&` in 
`createRequest` function, then all the caller will have the small changes to 
call `createRequest`. I quite agree with you to change this to `hashmap` to 
avoid the input error in format, and we can submit another patch after clean 
all http:get function.


- Yongqiao


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


On 八月 29, 2016, 10:47 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51493/
> ---
> 
> (Updated 八月 29, 2016, 10:47 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-4440
> https://issues.apache.org/jira/browse/MESOS-4440
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add the new 'query' parameter in createRequest method, which will be used by 
> Http GET method later.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 404196bb198c1ff958b55d72fb29c5fe92dba429 
>   3rdparty/libprocess/src/http.cpp 298bd460afdeccd18e201d8a505b961fd4cf3d3e 
> 
> Diff: https://reviews.apache.org/r/51493/diff/
> 
> 
> Testing
> ---
> 
> make & make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 51493: Add the query parameter in createRequest.

[haosdent huang]

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




3rdparty/libprocess/include/process/http.hpp (line 845)


I think use `hashmap` as type here would be better?


- haosdent huang


On Aug. 29, 2016, 10:47 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51493/
> ---
> 
> (Updated Aug. 29, 2016, 10:47 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-4440
> https://issues.apache.org/jira/browse/MESOS-4440
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add the new 'query' parameter in createRequest method, which will be used by 
> Http GET method later.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 404196bb198c1ff958b55d72fb29c5fe92dba429 
>   3rdparty/libprocess/src/http.cpp 298bd460afdeccd18e201d8a505b961fd4cf3d3e 
> 
> Diff: https://reviews.apache.org/r/51493/diff/
> 
> 
> Testing
> ---
> 
> make & make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>