> 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()) > > << "..."; > > ```
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. > 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? **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. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30911/#review73294 ----------------------------------------------------------- On Feb. 19, 2015, 2:54 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30911/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2015, 2:54 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 c7cc46e0183ea97013dd088a717da6c0e6ed5cf0 > src/common/resources.cpp 98371f6873482d0cdbefeb279b58ae6cc680a88f > src/master/allocator/mesos/hierarchical.hpp > 2680d6231927867d5a8d75cbc42b81d6c75fc7f2 > src/master/master.hpp 6a39df04514c756415354fae66c5835ada191c52 > src/master/validation.cpp acc35b25c93f2d3900d79c3070b1d681416ed66b > src/slave/slave.cpp ec7ec1356e745bb07484ae1755c9183b038043b3 > 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 > >
