mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 36037: Adding /call endpoint to Master
Date Tue, 07 Jul 2015 23:39:54 GMT


> On July 3, 2015, 12:29 a.m., Ben Mahler wrote:
> > I chatted with Isabel on IRC and asked her to break apart this change into more
bite-sized chunks, so that we can do smaller reviews and get things committed incrementally:
> > 
> > (1) Dummy /call handler on the master.
> > (2) Validation.
> > (3) Partial implementation of Call (i.e. parsing logic).
> > 
> > Each part can have its own tests. She will be discarding this review in favor of
smaller chunks, which we can commit incrementally. :)
> > 
> > I also asked her to:
> > 
> > (a) Punt on the constants and remove master/http_constants.hpp, since these constants
aren't adding value (CLOSE -> "close") for the added indirection, and our existing code
doesn't follow this pattern.
> > (b) Pull out the change to src/tests/mesos.hpp, since it is independent.
> 
> Marco Massenzio wrote:
>     All good.
>     However, I beg to disagree on this point:
>     >(a) Punt on the constants and remove master/http_constants.hpp, since these constants
aren't adding value (CLOSE -> "close") for the added indirection, and our existing code
doesn't follow this pattern.
>     
>     We *do* have a `constants.hpp` (and relative .cpp) file and I do believe it does
add value (I, for one, certainly appreciated having it when I did the JSON/ZK change ;) ):
for example, I've already seen the string `application/x-protobuf` typed up 10 times in just
two reviews: there is value in having an APPLICATION_PROTOBUF constant to:
>     
>     - avoid difficult-to-spot bugs to typos (`application/x-prolobuf`) that may only
surface at runtime in production;
>     - avoid typing the same stuff again and again (especially those of us using modern
IDEs can take advantage of code-completion ;) )
>     - this is anyway common standard good practice and would allow us to not having to
agonize too much in case we need to refactor something (say, at some point we want to use
`application/x-protobuf-binary` for whatever reason - there's only one place to do so; sure,
this is an unlikely example, but there may be cases where it may not be so far-fetched).
>     
>     Also, *not* doing it does not save (I think?) any effort in reviewing and/or committing,
so seems very low cost for a potential sizeable payoff.
>     
>     Ah, yes, and there's also the fact that hard-coded strings sprinkled all over the
code base are hard to maintain - I know, I've had to pick up the pieces at least twice in
the last 4 years ;)
>     
>     PS - I do agree that defining `const string CLOSE = "close"` may be pushing this
one step too far... but I'd like to retain it for those more commonly used strings.
> 
> Ben Mahler wrote:
>     I don't think we're in disagreement, I just want this to be punted so that we can
think carefully about how to improve 'Request' and 'Response' usage, rather than bundling
it in this code review. Doing more than one thing in a review tends to drag out the review,
and I didn't want Isabel to get distracted with this.
>     
>     So let's follow up! In particular, having http constants in master/http_constants.hpp
is strange because it isn't master specific (we have common/http.hpp for ones relevant to
many components in mesos, http.hpp for libprocess). Also, where possible, I'm hoping to avoid
the difficulty in header map manipulation entirely by providing typed members (there's a [TODO](https://github.com/apache/mesos/blob/0.23.0-rc1/3rdparty/libprocess/include/process/http.hpp#L107)
which briefly alludes to this). For example, `request.connection` could be an enum to capture
the possible connection types.

> I don't think we're in disagreement

Awesome! :)

> Doing more than one thing in a review tends to drag out the review,

Completely agree, I just thought that factoring out the constants at the outset was rather
uncontroversial and best done now rather than have to refactor later.

> having http constants in master/http_constants.hpp 

yep - I had actually missed that: do you have a better suggestion? (maybe `common/api_constants.hpp`
could be a better place/name? something else?)


> For example, request.connection could be an enum to capture the possible connection types.

Oh, yes - totally: typed enum >> string constant >> hard-coded string :)


- Marco


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


On July 2, 2015, 8:16 a.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36037/
> -----------------------------------------------------------
> 
> (Updated July 2, 2015, 8:16 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio,
and Vinod Kone.
> 
> 
> Bugs: MESOS-2860
>     https://issues.apache.org/jira/browse/MESOS-2860
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> Adding a call route with HTTP request header validations
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am a064d17 
>   src/master/http.cpp 2be613b 
>   src/master/http_constants.hpp PRE-CREATION 
>   src/master/http_constants.cpp PRE-CREATION 
>   src/master/master.hpp af83d3e 
>   src/master/master.cpp a7486d8 
>   src/master/validation.hpp 469d6f5 
>   src/master/validation.cpp 9d128aa 
>   src/tests/call_tests.cpp PRE-CREATION 
>   src/tests/mesos.hpp 9157ac0 
> 
> Diff: https://reviews.apache.org/r/36037/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


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