mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anand Mazumdar" <mazumdar.an...@gmail.com>
Subject Re: Review Request 37303: Moved scheduler library to http
Date Wed, 12 Aug 2015 04:12:41 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message