mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 53484: Introduced `RouteOptions` to support streaming requests.
Date Sat, 05 Nov 2016 01:37:32 GMT

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


Fix it, then Ship it!




How about updating the summary to say "Introduced options for HTTP routes, initially for streaming
requests."

In the ideal, we could just say "Introduced options for HTTP routes." which would mean we
only add the struct in this patch and not the streaming bit. Then, in the patch where we support
streaming we add the 'streaming' option to the struct here. This approach would be a bit cleaner
for posterity when looking at the committed patches, but it's up to you.


3rdparty/libprocess/include/process/process.hpp (line 262)
<https://reviews.apache.org/r/53484/#comment224864>

    I wonder if this should be 'streamingRequest' or Request::Type? As it stands I wonder
if it some might misinterpret it as being related to streaming responses (since that is more
common).
    
    Up to you.



3rdparty/libprocess/include/process/process.hpp (lines 279 - 285)
<https://reviews.apache.org/r/53484/#comment224865>

    How about using a default argument rather than an overload, since that expresses the intent:
if you don't provide us with RouteOptions, we will give you defaults.



3rdparty/libprocess/include/process/process.hpp (line 485)
<https://reviews.apache.org/r/53484/#comment224866>

    Since we define a notion of what the "default" options are, we can just remove the Option<>
here? Seems a bit simpler to reason about if there are always options, since "no options"
and "default options" are equivalent AFAICT.


- Benjamin Mahler


On Nov. 5, 2016, 1:33 a.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53484/
> -----------------------------------------------------------
> 
> (Updated Nov. 5, 2016, 1:33 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
>     https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This allows routes to specify configuration options. Currently, it
> only has one member `streaming` i.e, if the route supports request
> streaming. This also enables us to add more options in the future
> without polluting overloads.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/process.hpp f389d3d3b671e301a7ac911ad87ab13289e8c82f

>   3rdparty/libprocess/src/process.cpp ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 
> 
> Diff: https://reviews.apache.org/r/53484/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>


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