----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70851/#review215873 -----------------------------------------------------------
One concern I have with this approach is that, in my opinion, it misses an opportunity to decouple our internal representation of durations from the representation we expose in the APIs. It seems unnecessary to make a breaking change to the master's data persistence schema when we're introducing the change strictly to make APIs easier to consume. Here are a couple other approaches to consider: 1) Do not modify `DurationInfo`, but instead introduce a new message with the `seconds` and `nanoseconds` fields for use in user-facing APIs. We can deprecate existing uses of `DurationInfo` in the APIs (we can file a separate ticket for this work, don't need to do it immediately), and add new fields which make use of the new message. 2) Rename our stout `Duration` class. Is this something we will need to do before upgrading to proto 3 anyway, given the conflict with the `Duration` message in proto 3? In both of the proposals above, we would continue to use the existing `DurationInfo` message to persist durations to the registry and elsewhere. WDYT? - Greg Mann On June 13, 2019, 11:47 a.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70851/ > ----------------------------------------------------------- > > (Updated June 13, 2019, 11:47 a.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, and > Vinod Kone. > > > Repository: mesos > > > Description > ------- > > This adds a backwards incompatible addition to the DurationInfo > protobuf, which allows specifying duration in seconds instead > of nanoseconds. The new field is mutually exclusive with > nanoseconds (protobuf `oneof`). > > This adds a utility function to convert from DurationInfo objects > to the stout Duration class. All call sites which parse or read a > DurationInfo are updated to use this utility. > > A couple of tests are updated to specify seconds in DurationInfo. > > This change is backwards incompatible because the 'nanoseconds' field > has been changed to an optional field and can now be left blank in favor > of 'seconds'. This means, for example, a maintenance schedule specified > in seconds will not be recoverable by an older master. Or, a KillPolicy > specified in seconds will not be parsable by older agents. > > > Diffs > ----- > > include/mesos/mesos.proto 2b4f350815935220c2d2b0dd0e52346bc74c91d9 > include/mesos/type_utils.hpp 57b1893160dbe874aa9fec00a3d1b640b9c54906 > include/mesos/v1/mesos.hpp df67f64fc537819bf8607e6d6b4a478b544df69e > include/mesos/v1/mesos.proto bafc27499f810791700c4a30dcb1da33b6f31d2e > src/docker/executor.cpp f638e4b65155bcca1be36424b7061ea26a3d6ca3 > src/launcher/default_executor.cpp 5837cfa4deba557cae43112092ff24b97137951f > src/launcher/executor.cpp 38d82614ed82e8a6644334f0401cecdee6a025bf > src/launcher/fetcher.cpp 9cb81967459c7aa795a267fb7df218b528e43e64 > src/master/http.cpp 3cd7df228adb54a963821c23b4d1c26d33622ee7 > src/master/maintenance.cpp c07b815f04035eb6762f95a68fe99c5b49204ba9 > src/master/validation.cpp af2d04a46750e5d42e7d92c70ce685e64d646a3b > src/slave/http.cpp 69e6d74e8b113cc6c937f47df8984ff9a63e5bb4 > src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e > src/tests/check_tests.cpp d8a1a9b2791b61884fbc790658ec39ac403aa68b > src/tests/command_executor_tests.cpp > 02ae250a599043f30ef5879ce528815e0ded5d3d > src/tests/master_maintenance_tests.cpp > 303eaaac2dc3966bc16b7eb8de11c3ccf960f1bb > src/tests/slave_tests.cpp c2035976713abb31b3646c0d23771fa40df93271 > > > Diff: https://reviews.apache.org/r/70851/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Joseph Wu > >
