This is an automatically generated e-mail. To reply, visit:

looks like the SchedulerTest.Subscribe test failed?

src/common/http.hpp (line 66)

    // Deserializes a string message into a protobuf message
    // based on the HTTP content type

src/common/http.hpp (line 67)

    hmm. this is not as generic as serialize. can't this be templated?
    template <typename T>
    Try<T> deserialize(...);

src/common/http.hpp (line 73)

    this helper seems weird. i would rather have isOK()and isAccepted() helpers.
    anyway, this seems to be used only once. do we even need a helper?

src/common/http.cpp (line 86)

    add a default case please, in case someone adds a new supported contentType 

src/common/http.cpp (line 101)

    ditto. add a default case.

src/scheduler/scheduler.cpp (line 211)

    why new synchronous function (_send()) here instead of directly plopping 
the code here?
    we prefer procedural programming style, i.e., we don't introduce new 
methods/functions unless it's necesary.


    this should've been in the patch that removed pid related stuff.

src/scheduler/scheduler.cpp (line 296)

    s/call type Subscribe/Subscribe call/

src/scheduler/scheduler.cpp (line 314)


src/scheduler/scheduler.cpp (line 315)

    handling error like this loses the context about the call that caused the 
error, no?
    i would just do a .onAny() instead of .then() and .onFailed().

src/scheduler/scheduler.cpp (line 320)

    you should make sure subscribe got an OK and everything else got Accepted. 
for that this method needs to take Call as argument.

src/scheduler/scheduler.cpp (line 322)

    ditto. this should include which call/request failed.

src/scheduler/scheduler.cpp (line 334)

    This needs a comment. Why would there be an existing reader? Why is an 
existing reader not closed as soon as a subscribe call is made?

src/scheduler/scheduler.cpp (line 345)

    are all other response types (non PIP) ok to receive?

src/scheduler/scheduler.cpp (lines 358 - 359)

    Just say that Events stream filed and include the failure message.

src/scheduler/scheduler.cpp (line 375)

    ditto. please don't create new functions unless there is re-use.

src/scheduler/scheduler.cpp (line 381)

    When does this happen? Add a comment.
    Also, sending a "subscribe with master.." with every error event seems a 
bit redundat? Can we just add documentation to the ERROR event in the protobuf 
saying so? Or are there events where we dont want schedulers to oversubscribe?

src/scheduler/scheduler.cpp (lines 392 - 393)

    When does this happen?

src/scheduler/scheduler.cpp (lines 406 - 407)

    include the error please.

src/scheduler/scheduler.cpp (line 423)

    make this a LOG(WARNING)

src/scheduler/scheduler.cpp (lines 483 - 490)

    process = new MesosProcess(

- Vinod Kone

On Aug. 10, 2015, 4:43 p.m., Anand Mazumdar wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37303/
> -----------------------------------------------------------
> (Updated Aug. 10, 2015, 4:43 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 a8699a7514892a548eb03f9dcb6a1198ce518830 
> Diff: https://reviews.apache.org/r/37303/diff/
> Testing
> -------
> make check
> Thanks,
> Anand Mazumdar

Reply via email to