From reviews-return-4447-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Sat Jul 4 01:56:05 2015 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id EBE87184D2 for ; Sat, 4 Jul 2015 01:56:05 +0000 (UTC) Received: (qmail 52245 invoked by uid 500); 4 Jul 2015 01:56:05 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 52217 invoked by uid 500); 4 Jul 2015 01:56:05 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 52200 invoked by uid 99); 4 Jul 2015 01:56:05 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 04 Jul 2015 01:56:05 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 50078B2E99; Sat, 4 Jul 2015 01:56:04 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============0047012010090259394==" MIME-Version: 1.0 Subject: Re: Review Request 36037: Adding /call endpoint to Master From: "Marco Massenzio" To: "Anand Mazumdar" , "Benjamin Hindman" , "Ben Mahler" , "Vinod Kone" , "Marco Massenzio" Cc: "Isabel Jimenez" , "mesos" Date: Sat, 04 Jul 2015 01:56:04 -0000 Message-ID: <20150704015604.13307.25770@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: "Marco Massenzio" X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/36037/ X-Sender: "Marco Massenzio" References: <20150703002959.13307.66897@reviews.apache.org> In-Reply-To: <20150703002959.13307.66897@reviews.apache.org> Reply-To: "Marco Massenzio" X-ReviewRequest-Repository: mesos-incubating --===============0047012010090259394== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > 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 > > --===============0047012010090259394==--