----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74875/#review226268 -----------------------------------------------------------
Fix it, then Ship it! I'll take care of these for you and land it! src/linux/cgroups2.cpp Lines 75 (patched) <https://reviews.apache.org/r/74875/#comment314500> this needs an <iterator> include src/linux/cgroups2.cpp Lines 88 (patched) <https://reviews.apache.org/r/74875/#comment314502> this is const as well? src/linux/cgroups2.cpp Lines 93 (patched) <https://reviews.apache.org/r/74875/#comment314504> this is copying the vector src/linux/cgroups2.cpp Lines 116-131 (patched) <https://reviews.apache.org/r/74875/#comment314508> see comment below, this can just be a parse function rather than a read, and that way State provides parsing and stringification and the caller can compose the reading and writing to files (also winds up being more unit testable) src/linux/cgroups2.cpp Lines 127 (patched) <https://reviews.apache.org/r/74875/#comment314506> split needs a <stout/strings.hpp> include src/linux/cgroups2.cpp Lines 128 (patched) <https://reviews.apache.org/r/74875/#comment314509> move here as well? (note this doesn't matter since the strings are small and therefore not on the heap, but I saw you did it below as well) src/linux/cgroups2.cpp Lines 134-135 (patched) <https://reviews.apache.org/r/74875/#comment314503> what does m here represent? src/linux/cgroups2.cpp Lines 138 (patched) <https://reviews.apache.org/r/74875/#comment314510> nit: keep the &'s consistent (against the type) src/linux/cgroups2.cpp Lines 262-267 (patched) <https://reviews.apache.org/r/74875/#comment314505> why the extra function indirection here..? src/linux/cgroups2.cpp Lines 273-281 (patched) <https://reviews.apache.org/r/74875/#comment314507> hm.. it's a bit weird that we need another write for this specifically, looks more like we can just use the existing one: ``` return write(cgroup, control::SUBTREE_CONTROLLERS, stringify(*control)); ``` same for read here, it's more composable for the state to be able to parse, but not actually do the read (more unit testable as well): ``` Try<string> contents = cgroups2::read( cgroup, cgroups2::control::SUBTREE_CONTROLLERS); if (contents.isError()) { return Error(contents.error()); } State control = State::parse(*contents); control->enable(subsystems); return cgroups2::write( cgroup, control::SUBTREE_CONTROLLERS, stringify(control)); ``` src/tests/containerizer/cgroups2_tests.cpp Lines 41 (patched) <https://reviews.apache.org/r/74875/#comment314512> hm.. we don't need the extra boolean? we alrady have the state inside `mounted`? src/tests/containerizer/cgroups2_tests.cpp Lines 43-47 (patched) <https://reviews.apache.org/r/74875/#comment314513> use `ASSERT_` instead of `EXPECT_` where you need the test to stop on failure.. in particular here you don't want to derefrence the Try if it's none (will crash), same case below with the other Try src/tests/containerizer/cgroups2_tests.cpp Lines 45 (patched) <https://reviews.apache.org/r/74875/#comment314511> hm.. not sure if we want the test to be mounting it, but let's keep it for now and revisit if needed - Benjamin Mahler On Feb. 28, 2024, 5:09 p.m., Devin Leamy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74875/ > ----------------------------------------------------------- > > (Updated Feb. 28, 2024, 5:09 p.m.) > > > Review request for mesos. > > > Repository: mesos > > > Description > ------- > > Creates an interface to "cgroups.controllers" to obtain a set of > the subsystems available on a host. > > > Diffs > ----- > > src/linux/cgroups2.hpp 0c54e464b26c2a72b5efc8c461066888c7442257 > src/linux/cgroups2.cpp ec7ba9e4028276ebcb0c40be18b8975bb03c217e > src/tests/containerizer/cgroups2_tests.cpp > 4981e9588b5f327e84172a31fb722484ab33ff18 > > > Diff: https://reviews.apache.org/r/74875/diff/2/ > > > Testing > ------- > > > Thanks, > > Devin Leamy > >
