> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > Thanks Anand, mostly thinking we can clean up the read logic if we have a > > struct to capture the reader / decoder.
Isn't it much more simpler here? It's just a one liner "if" check to check if the reader is reader is not None and != for stale reader check. Here is the code snipped I have used: if (!reader.isSome() || reader.get() != oldReader) { > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, lines 353-355 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line353> > > > > How about: > > > > ``` > > Received a '500 Internal Error' for SUBSCRIBE call: Body of request > > ``` > > > > Remember that we don't use periods in logging messages Sounds good to me. I suppose we don't use periods to terminate logging messages ? > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, lines 390-397 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line390> > > > > Why does master disconnection send an Error event? This can occur if > > the master crashes, fails over, etc. So the scheduler should not see an > > error for this? How else would you communicate to the scheduler that it needs to subscribe again in case of master disconnection/failover ? Feel free to re-open in case I missed something > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 328 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line328> > > > > Can you flip this comparison? Why would you want to do that ? process::http::statuses[200] is a constant and helps identify bugs like if (a = 5) by keeping the constant check on the left ? Seems like I am missing something here > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 111 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line111> > > > > This is just for testing right? Might be nice to expose as a separate > > constructor with a note that it's for testing. Not quite. I intend to add an constructor to Mesos::Mesos i.e. the wrapper exposed class to the user later that would be used for testing. This is just the internal implementation class. > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 117 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line117> > > > > Hm.. can we avoid the 'headers' variable? Seems clearer to keep it > > local to an individual request for now. Why ? This is anyways just the internal implementation class. Since we have to do that for every request we make. Why not just save it as a constant member variable ? > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, lines 219-221 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line219> > > > > This is required, so how about just saying: > > > > ``` > > // Each subscription requires a new connection. > > disconnect(); > > ``` Sounds good. Done > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/common/http.hpp, lines 80-81 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037897#file1037897line80> > > > > value.get() need to be called only if value.isSome() Fixed , my bad. > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, lines 222-223 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line222> > > > > Can we call this `disconnect`? Any reason to not clear the reader > > within the function rather than in the caller? Fixed > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 323 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line323> > > > > Need to deal with isFailed case before you call .get() as well. Fixed > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 329 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line329> > > > > CHECK_EQ? Fixed > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 435 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line435> > > > > Can we call this 'disconnect'? Fixed > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 437 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line437> > > > > space here :) I wish we as a community can get adoption for clang format soon. These things should be handled by a tool rather then someone spending time reviewing them :( > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 439 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line439> > > > > Can we avoid saying "reader" in these messages? Let's just say that the > > connection was already closed, since the users of this library don't care > > about the implementation detail of there being a Reader. Fixed > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 383 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line383> > > > > Failed to decode the stream of events: ... Fixed > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, lines 400-401 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line400> > > > > s/chunk/event/ > > > > How about: > > > > ``` > > Failed to de-serialize event sent by master: ... > > ``` Fixed > On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote: > > src/common/http.hpp, lines 51-53 > > <https://reviews.apache.org/r/37303/diff/3/?file=1037897#file1037897line51> > > > > Can't we just have support for stringifying these? > > > > e.g. stringify(ContentType::PROTOBUF) == "APPLICATION_PROTOBUF" > > > > Or is there something I'm missing? Good point. Added. - Anand ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37303/#review95036 ----------------------------------------------------------- On Aug. 11, 2015, 10:18 p.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37303/ > ----------------------------------------------------------- > > (Updated Aug. 11, 2015, 10:18 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Bugs: MESOS-2552 > https://issues.apache.org/jira/browse/MESOS-2552 > > > Repository: mesos > > > Description > ------- > > This changes moves scheduler library to http. The change to remove auth + > install,receive handlers are in other reviews. > > > Diffs > ----- > > src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 > src/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50 > src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 > > Diff: https://reviews.apache.org/r/37303/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Anand Mazumdar > >