----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53994/#review157134 -----------------------------------------------------------
Fix it, then Ship it! Looks good, just some minor comments. src/slave/http.cpp (lines 335 - 356) <https://reviews.apache.org/r/53994/#comment227560> hmm, we should kill this block and use `deserialize()` in `src/common/http.hpp` directly. It seems that we never cleaned this block up after implementing `deserialize()`. This would ensure that we keep the deserialization logic in one place. Note that this applies to the other handlers on the master/agent too and we might want to do a sweep later. src/slave/http.cpp (lines 354 - 355) <https://reviews.apache.org/r/53994/#comment227557> Can it ever go inside this `else` block provided that we already checked for content type being JSON/PROTOBUF before? I would just put an `UNREACHABLE()` here. Also, we might not need this `else` block if you replace this block with `deserialize()` as per my earlier comment. src/slave/http.cpp (line 390) <https://reviews.apache.org/r/53994/#comment227566> Do you need to implicitly capture everything or can we be explicit here? - Anand Mazumdar On Nov. 28, 2016, 8:56 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53994/ > ----------------------------------------------------------- > > (Updated Nov. 28, 2016, 8:56 p.m.) > > > Review request for mesos, Anand Mazumdar and Benjamin Mahler. > > > Bugs: MESOS-6473 > https://issues.apache.org/jira/browse/MESOS-6473 > > > Repository: mesos > > > Description > ------- > > Note that this change only updates the handler to correctly > handle non-streaming calls given it receives `PIPE` requests. > Handling streaming calls will come later. > > > Diffs > ----- > > src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f > src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb > src/slave/slave.cpp 521f08d59cd78f9089d58cd3294f0ee4a099cd7f > > Diff: https://reviews.apache.org/r/53994/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
