> On Aug. 15, 2019, 5:14 a.m., James Peach wrote: > > src/slave/state.cpp > > Lines 819 (patched) > > <https://reviews.apache.org/r/71285/diff/1/?file=2160811#file2160811line819> > > > > `state::read` can already return `None()`, so what do you think about > > letting it return `None()` when the file to read isn't present? > > > > I think that this would improve the code in a number of places, but > > it's a large enough change that a separate review would be better.
After looking at the callsites, I'm not convinced that returning `None()` when the file doesn't exist will make them simpler. We'll still need checks for `result.isNone()`, and I think the code is actually a bit more readable to have explicit checks for `os::exists()` instead, rather than hiding this information behind the interface of `state::read()`. Let me know what you think, maybe I'm missing something. - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71285/#review217215 ----------------------------------------------------------- On Aug. 19, 2019, 10:54 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71285/ > ----------------------------------------------------------- > > (Updated Aug. 19, 2019, 10:54 p.m.) > > > Review request for mesos, Gastón Kleiman, James Peach, and Joseph Wu. > > > Bugs: MESOS-9875 > https://issues.apache.org/jira/browse/MESOS-9875 > > > Repository: mesos > > > Description > ------- > > Fixed recovery of agent resources and operations after crash. > > > Diffs > ----- > > src/slave/paths.hpp e077587fd02bd8e35fee7ce12ae436e3dca25e47 > src/slave/paths.cpp 28a7cf9f9c70fb31eeefe2e823cd7e19ffcf126a > src/slave/slave.cpp 1d0ec9d2428c3ffa28ad3e960b74f171013cf0c2 > src/slave/state.cpp cd3fac72dd57da21ed5ac46b17066531af26d42a > > > Diff: https://reviews.apache.org/r/71285/diff/2/ > > > Testing > ------- > > Tested manually by doing the following: > > 1) Run a master > 2) Run an agent with statically reserved resources for use in the following > step > 3) Run the persistent volume example framework to create a volume on the > agent, which leads to checkpointing of resources > 4) Use `chattr -R +i /agent/volumes/dir` to make the agent's persistent > volume directory immutable > 5) Run the persistent volume example framework again; it will fail and the > agent will crash > 6) Restart the agent; confirm that it continues to crash > 7) Use `chattr -R -i /agent/volumes/dir` to remove the immutable attribute > 8) Restart the agent; confirm that it recovers successfully, with the > persistent volume created on disk > 9) Run the persistent volume example framework again; it should succeed > > > Thanks, > > Greg Mann > >