mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 64096: Implemented the `OfferOperationStatusUpdateManager`.
Date Wed, 06 Dec 2017 20:29:33 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64096/#review192999
-----------------------------------------------------------




src/status_update_manager/offer_operation.hpp
Lines 17 (patched)
<https://reviews.apache.org/r/64096/#comment271452>

    Let's include the directory in this (see the headers in 'resource_provider/', for example:
    
    ```
    #ifndef __STATUS_UPDATE_MANAGER_OFFER_OPERATION__HPP__
    ```
    
    At the end of the file as well



src/status_update_manager/offer_operation.hpp
Lines 39-42 (patched)
<https://reviews.apache.org/r/64096/#comment271466>

    Looks like we usually indent 4 spaces in these cases? Please confirm and update if appropriate,
here and elsewhere.



src/status_update_manager/offer_operation.hpp
Lines 47-48 (patched)
<https://reviews.apache.org/r/64096/#comment271469>

    To ensure we don't have duplicate status update managers attempting to access the same
streams, we could also delete the copy constructor and assignment operator.



src/status_update_manager/offer_operation.hpp
Lines 48 (patched)
<https://reviews.apache.org/r/64096/#comment271467>

    This doesn't need to be `virtual` since we don't expect to use this as a base class. Instead,
let's do:
    
    ```
    ~OfferOperationStatusUpdateManager() override;
    ```
    
    `override` will ensure that we get a compile-time error if this method fails to override
a base class destructor in the future.



src/status_update_manager/offer_operation.hpp
Lines 60 (patched)
<https://reviews.apache.org/r/64096/#comment271468>

    s/@return Whether/Returns whether/



src/status_update_manager/offer_operation.hpp
Lines 97-99 (patched)
<https://reviews.apache.org/r/64096/#comment271470>

    We don't usually indent the lines after the NOTE.



src/status_update_manager/offer_operation.cpp
Lines 57-64 (patched)
<https://reviews.apache.org/r/64096/#comment271471>

    Indentation. Please fix here and elsewhere.



src/status_update_manager/offer_operation.cpp
Lines 72 (patched)
<https://reviews.apache.org/r/64096/#comment271479>

    We should really only do something like this in test code (a `.get()` without a CHECK
or `if` guard). Adding the CHECK will get us a stack trace if things go wrong. Instead, here
let's do:
    
    ```
    Try<UUID> operationUuid = UUID::fromBytes(update.operation_uuid());
    CHECK_SOME(operationUuid);
    
    return dispatch(
        process.get(),
        &StatusUpdateManagerProcess<
            UUID,
            OfferOperationStatusUpdateRecord,
            OfferOperationStatusUpdate>::update,
        update,
        operationUuid.get(),
        checkpoint);
    ```



src/status_update_manager/offer_operation.cpp
Lines 76-79 (patched)
<https://reviews.apache.org/r/64096/#comment271473>

    Could use a typedef to clean this up a bit - I'll leave it up to you.



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 52 (patched)
<https://reviews.apache.org/r/64096/#comment271496>

    Could you add a comment here explaining the motivation for this class?



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 56-58 (patched)
<https://reviews.apache.org/r/64096/#comment271474>

    One more newline here.



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 62-63 (patched)
<https://reviews.apache.org/r/64096/#comment271475>

    const ref?



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 104 (patched)
<https://reviews.apache.org/r/64096/#comment271501>

    s/statusUpdatesProcessor/statusUpdateProcessor/
    
    seems more consistent?



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 108 (patched)
<https://reviews.apache.org/r/64096/#comment271505>

    Let's s/SUM/status update manager/ throughout this file.
    
    I appreciate the desire for brevity, but I would prefer to make the comments maximally
comprehensible to a reader who drops in to look at just a single test in the future, without
lots of context.



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 108-109 (patched)
<https://reviews.apache.org/r/64096/#comment271507>

    The "until" in this comment seems to imply that we are testing retries as well.
    
    Perhaps:
    "This test verifies that the status update manager will not retry an update after it has
been acknowledged."



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 145-146 (patched)
<https://reviews.apache.org/r/64096/#comment271510>

    Ditto on the wording of this comment.



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 166 (patched)
<https://reviews.apache.org/r/64096/#comment271513>

    Is there a reason not to pause the clock at the beginning of every test case? Perhaps
that could even go into the fixture?



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 178 (patched)
<https://reviews.apache.org/r/64096/#comment271517>

    If you decide to pause for the entirety of each test, perhaps this final `resume` could
go into the fixture as well.



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 244 (patched)
<https://reviews.apache.org/r/64096/#comment271520>

    Let's declare a `FrameworkID frameworkId` above and pass it here. We can also use it in
the `cleanup` call below.



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 297-303 (patched)
<https://reviews.apache.org/r/64096/#comment271523>

    Is the purpose of this cleanup call to clear the status update manager state? If so, let's
say that explicitly in the comment.
    
    It would be more compelling to actually instantiate a new status update manager and then
recover from the previous state, but if that's difficult this works as well.



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 346 (patched)
<https://reviews.apache.org/r/64096/#comment271525>

    Since the framework ID isn't strictly necessary here, it might be nice to omit it and
have another way to clear the manager state.
    
    Perhaps the fixture could have a `resetStatusUpdateManager()` method which resets the
SUM member?
    
    Here and elsewhere.



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 369 (patched)
<https://reviews.apache.org/r/64096/#comment271526>

    s/returns//



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 408-410 (patched)
<https://reviews.apache.org/r/64096/#comment271527>

    s/but before any data/but the status update manager crashed before any data/



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 428 (patched)
<https://reviews.apache.org/r/64096/#comment271528>

    s/returns//



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 670-671 (patched)
<https://reviews.apache.org/r/64096/#comment271531>

    s/ignored/acknowledged/



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 723-724 (patched)
<https://reviews.apache.org/r/64096/#comment271533>

    s/ignored/acknowledged/



src/tests/offer_operation_status_update_manager_tests.cpp
Lines 778-780 (patched)
<https://reviews.apache.org/r/64096/#comment271534>

    Is this record "corrupted"? Looks to me like it's a duplicate?


- Greg Mann


On Dec. 4, 2017, 11:08 p.m., Gaston Kleiman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64096/
> -----------------------------------------------------------
> 
> (Updated Dec. 4, 2017, 11:08 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8197
>     https://issues.apache.org/jira/browse/MESOS-8197
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This class will handle the offer operation status updates generated by
> the agent and by resource providers.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt a76ba1e73a3480baba2661b2e680814204df4d3e 
>   src/Makefile.am 05e8b950a3ee13f7b2e8af9416495f2827138449 
>   src/status_update_manager/offer_operation.hpp PRE-CREATION 
>   src/status_update_manager/offer_operation.cpp PRE-CREATION 
>   src/tests/CMakeLists.txt b74fbb9bb19f9d73569b2e83e34d8959ad3aabd4 
>   src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64096/diff/9/
> 
> 
> Testing
> -------
> 
> This patch addes new tests that passed 5000 times on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>


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