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