> On Feb. 20, 2015, 6:30 p.m., Jie Yu wrote: > > src/slave/slave.cpp, lines 1301-1318 > > <https://reviews.apache.org/r/30911/diff/3/?file=868710#file868710line1301> > > > > Can we simply the logic here to be: > > ``` > > Resources resources = task.resources(); > > if (task.has_executor()) { > > resources += task.executor().resources(); > > } > > > > CHECK(checkpointedResources.contains(resources.persistentVolumes()) > > << "..."; > > ``` > > Michael Park wrote: > We could, the only thing for me is that the error message wouldn't be as > descriptive. We would say something like "Unknown persistent volume for task > or executor of framework <FrameworkID>". Whereas right now we report the > `volume` and whether the issue is with task resources or executor resources > along with its ID.
Good point, dropping. > On Feb. 20, 2015, 6:30 p.m., Jie Yu wrote: > > include/mesos/resources.hpp, lines 148-150 > > <https://reviews.apache.org/r/30911/diff/3/?file=868705#file868705line148> > > > > We don't use snake_case for function names. Please > > ``` > > s/persitent_volumes/persistentVolumes > > ``` > > Jie Yu wrote: > I am a little hesitate if we want to introduce this helper or not. In > many cases, the code is much simpler if we do > ``` > foreach (const Resource& resource, task.resources()) { > if (!resource.isPersistentVolume()) { > continue; > } > ... > } > ``` > > Also, looks like the following code is not too bad as well. > ``` > resources.filter(Resources::isPersistentVolume) > ``` > > What do you think? > > Michael Park wrote: > **motivation**: I introduced this one because you wanted to keep > `reserved(role)` and `unreserved()` because of their convenience, and I found > that `persistentVolumes()` is actually used more than those across the > codebase. > > **readability**: I think it looks "less readable" only because we need to > explicitly construct the `Resources` object like: > `Resources(task.resources()).persistentVolumes()`, which I tried to avoid by > making `filter` be a non-member function, but we moved away from the > non-member approach. All I'm saying is that whatever arguments against > `persistentVolumes()` goes against `unreserved()` as well. For example, if > `resources.filter(Resources::isPersistentVolume)` is not too bad, > `resources.filter(Resources::isUnreserved)` can't be too bad either. > > **convenience factor**: we use `reserved(role)` 6 occurences, all in > tests. `unreserved()` occurs 9 times in > `src/master/allocator/mesos/hierarchical.hpp` but that needs to be changed to > `needCheckpointing` anyway, and 3 times in tests. `persistentVolumes` occur 4 > times in our source and once in a test. > > In general, I'm indifferent here as long as things are consistent. You are right! I've been back and forth, my bad. I think we can leave this as it is right now. My thought was, if we ever want to introduce more and more meta data in Resource, we would endup having too many such function. For example, we might need staticallyReserved(role), dynamicallyReserved(role), ... Because of that, probably we should just switch to use the filter mechenism consistently in the code. But we can do this refactor later. Feel free to ignore this comment. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30911/#review73294 ----------------------------------------------------------- On Feb. 21, 2015, 3:57 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30911/ > ----------------------------------------------------------- > > (Updated Feb. 21, 2015, 3:57 a.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and > Vinod Kone. > > > Bugs: MESOS-2348 > https://issues.apache.org/jira/browse/MESOS-2348 > > > Repository: mesos > > > Description > ------- > > See [JIRA Ticket](https://issues.apache.org/jira/browse/MESOS-2348). > > > Diffs > ----- > > include/mesos/resources.hpp c242bcc29c490841354d6fdc8d0de16eeea602ed > src/common/resources.cpp 625922dd1abe2420736867c6043ca39be4e526ef > src/master/allocator/mesos/hierarchical.hpp > 2680d6231927867d5a8d75cbc42b81d6c75fc7f2 > src/master/master.hpp a466f9299f116e1c3aae6fc79c73c9c165383769 > src/master/validation.cpp cd1052adc877cf698f5f627eef6ff4008aa573d5 > src/slave/slave.cpp 0374ca061cb53dc7e3338366aa41b1c15fb3231c > src/tests/hierarchical_allocator_tests.cpp > eeecfb64540b16666915074aaffaa5d506b203bc > src/tests/resources_tests.cpp 3f98782fd437dba808d720bf8e9b94b8fa7e0feb > > Diff: https://reviews.apache.org/r/30911/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Michael Park > >
