> 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
> 
>

Reply via email to