> On June 13, 2019, 5:50 a.m., Greg Mann wrote: > > 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?
Having a translation layer for our APIs would be pretty nice. Something akin to our existing evolve/devolve methods. I'd imagine that if we had a pure translation layer, we could get to a point where the master and agent (i.e. master.cpp and slave.cpp) would not even need to link to V1 protos. I don't know what to call a user facing duration object though... since Duration and DurationInfo are already taken :D - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70851/#review215873 ----------------------------------------------------------- On June 13, 2019, 4: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, 4: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 > >
