> On Feb. 7, 2013, 10:39 p.m., Benjamin Hindman wrote: > > include/mesos/mesos.proto, lines 251-252 > > <https://reviews.apache.org/r/9094/diff/5/?file=254441#file254441line251> > > > > Are these total times or since the last snapshot? If so, what's the > > interval or do I need to have a previous ResourceUsage to see it's > > timestamp?
These are the total times, I'll document that so that it's clear. > On Feb. 7, 2013, 10:39 p.m., Benjamin Hindman wrote: > > src/files/files.cpp, line 104 > > <https://reviews.apache.org/r/9094/diff/5/?file=254443#file254443line104> > > > > Yeah, I don't think we want this to propagate. I'll remove the CHECKs since we can think about this later, but we should definitely be looking at the result of route! > On Feb. 7, 2013, 10:39 p.m., Benjamin Hindman wrote: > > src/slave/isolation_module.hpp, line 22 > > <https://reviews.apache.org/r/9094/diff/5/?file=254448#file254448line22> > > > > Just curious, what did you need this for? Maybe a comment? Artifact. > On Feb. 7, 2013, 10:39 p.m., Benjamin Hindman wrote: > > src/slave/monitor.cpp, line 83 > > <https://reviews.apache.org/r/9094/diff/5/?file=254452#file254452line83> > > > > Maybe it should propagate until we fix it? I'm not going to propagate it through this review, but it's definitely something we should figure out. > On Feb. 7, 2013, 10:39 p.m., Benjamin Hindman wrote: > > third_party/libprocess/src/statistics.cpp, lines 343-348 > > <https://reviews.apache.org/r/9094/diff/5/?file=254459#file254459line343> > > > > How does the added output help us debug? IIUC, if one of these CHECKs > > fires there is a serious bug, regardless of the context and name. Am I > > missing something? Removed. I had added these when trying to fix a bug uncovered by my tests. > On Feb. 7, 2013, 10:39 p.m., Benjamin Hindman wrote: > > src/slave/monitor.cpp, lines 161-162 > > <https://reviews.apache.org/r/9094/diff/5/?file=254452#file254452line161> > > > > A comment on why you're not using const& here would be nice. I'm going to just bite the bullet and use const& here at the cost of the code being a bit uglier. > On Feb. 7, 2013, 10:39 p.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, line 318 > > <https://reviews.apache.org/r/9094/diff/5/?file=254456#file254456line318> > > > > This needs to be a flag (you can keep the constant if you like, but we > > should really only be referencing 'flag.resource_collection_interval'). In > > the long term we probably don't need to put constants that are only being > > referenced by flags in constants.hpp/cpp since it might be a source of > > confusion. Done, this is now a slave flag, and the constant is limited to the flag file itself rather than constants.hpp. > On Feb. 7, 2013, 10:39 p.m., Benjamin Hindman wrote: > > src/slave/monitor.cpp, line 160 > > <https://reviews.apache.org/r/9094/diff/5/?file=254452#file254452line160> > > > > So I get that you don't need this to be a method of > > ResourceMonitorProcess, but I've tended to like putting the continuation > > function immediately after the other function for readability reasons. That > > might mean having a declaration at the top of the file in order to > > reference it in ResourceMonitorProcess::usage. > On Feb. 7, 2013, 10:39 p.m., Benjamin Hindman wrote: > > src/slave/monitor.cpp, line 65 > > <https://reviews.apache.org/r/9094/diff/5/?file=254452#file254452line65> > > > > I like the notion of the 'expose' verb, but I feel like I read it > > better as 'export'. The former makes it seem like I'm trying to show > > something, the latter makes it seem like I'm trying to ship it somewhere > > (as in, ship it over to statistics). Also, this is kind of like a macro, > > and the implementation doesn't look like it needs to be a method but could > > just be a function. I agree and originally named this 'export' as well, but 'export' is a reserved keyword! So expose was the best synonym I could think of. Made this a function instead. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9094/#review16276 ----------------------------------------------------------- On Feb. 1, 2013, 1:12 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9094/ > ----------------------------------------------------------- > > (Updated Feb. 1, 2013, 1:12 a.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Description > ------- > > This is the meat of the resource monitoring design. > > -Added a ResourceUsage protobuf. > -Added a ResourceMonitor process that periodically hits the isolation module > for usage information. > -Usage is exported to the Statistics process. > -Usage is available via a JSON endpoint. > > Implementation of the isolation module code will follow in subsequent reviews. > > > This addresses bug MESOS-324. > https://issues.apache.org/jira/browse/MESOS-324 > > > Diffs > ----- > > include/mesos/executor.hpp 0ea70528a74bb9ba7d2aaac85d2ff85928363869 > include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801 > src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66 > src/files/files.cpp 60b567eb62e84ccc99b0b1978733a0ea96560813 > src/slave/cgroups_isolation_module.hpp > 669efa14ba2603764aa68ae19a44e79dbfdec192 > src/slave/cgroups_isolation_module.cpp > 63cefc33cf34eebb82db5d8448b751be8652fa36 > src/slave/constants.hpp ddf02570caf3793106b3c48e158a5bb48c1ae80c > src/slave/constants.cpp 1735a6b55a93e6537a5a119e5345961f3d84a000 > src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3 > src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4 > src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c > src/slave/monitor.hpp PRE-CREATION > src/slave/monitor.cpp PRE-CREATION > src/slave/process_based_isolation_module.hpp > f1817192582e3646f8dcf17934ba7998829e8fd6 > src/slave/process_based_isolation_module.cpp > 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5 > src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039 > src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018 > src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45 > third_party/libprocess/include/process/statistics.hpp > 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9 > third_party/libprocess/src/statistics.cpp > 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f > third_party/libprocess/src/tests/statistics_tests.cpp > 0aaab3526618171c7cfbd11d40d614344bcbfd0a > > Diff: https://reviews.apache.org/r/9094/diff/ > > > Testing > ------- > > make check > > Although I didn't add tests, I've manually tested end-to-end with my future > reviews. > > > Thanks, > > Ben Mahler > >
