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