> On Dec. 6, 2017, 12:29 p.m., Greg Mann wrote: > > src/status_update_manager/offer_operation.hpp > > Lines 48 (patched) > > <https://reviews.apache.org/r/64096/diff/9/?file=1909289#file1909289line48> > > > > 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.
I removed the `virtual` modifier. This is a base clase, so `override` doesn't make sense... should we make the class `final`? > On Dec. 6, 2017, 12:29 p.m., Greg Mann wrote: > > src/tests/offer_operation_status_update_manager_tests.cpp > > Lines 778-780 (patched) > > <https://reviews.apache.org/r/64096/diff/9/?file=1909292#file1909292line778> > > > > Is this record "corrupted"? Looks to me like it's a duplicate? The file should contain only `OfferOperationStatusUpdateRecord` messages. The test writes a `OfferOperationStatusUpdate` to the end of the file, so that the recovery process encounters an error. Added a comment describing this. - Gaston ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64096/#review192999 ----------------------------------------------------------- On Dec. 6, 2017, 5:22 p.m., Gaston Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64096/ > ----------------------------------------------------------- > > (Updated Dec. 6, 2017, 5:22 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 35a602d2afb3a1e6ef76a0b0df2628ce5493dc81 > 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 92db731a81303f0d1d95dfe0b80a0a512e165445 > src/tests/offer_operation_status_update_manager_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/64096/diff/10/ > > > Testing > ------- > > This patch addes new tests that passed 5000 times on GNU/Linux. > > > Thanks, > > Gaston Kleiman > >
