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 Sat, 04 Jul 2015 01:56:04 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.

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.


- 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