> On Feb. 3, 2016, 10:45 p.m., Michael Park wrote: > > src/tests/resources_tests.cpp, line 957 > > <https://reviews.apache.org/r/42751/diff/2/?file=1231620#file1231620line957> > > > > We used to have a `Resources::size()` function which essentially did > > this, but intentionally removed it so that people don't rely on number of > > `Resource` instances. Is there a reason why we want to check for this? > > > > Here and below. > > Neil Conway wrote: > The # of `Resource` instances is part of the public API of `Resources` > (e.g., clients can iterate over every `Resource`). If it is part of the > public API, it seems like something it would be worth testing. > > In this particular case, it doesn't matter that much, but in other test > cases (e.g., `AdditionDynamicallyReservedWithDistinctLabels`) it seems useful > to check.
Ok, synced with Jie on this as well. Let's re-introduce `Resources::size()` and use that instead. I agree that the # of `Resource` instances is part of the public API since as you say, one can iterate and count. However, this still does not mean that such iterator math works. For example, we could internally store the `Resource` objects in a `std::set` (or any other data structure that does not guarantee contiguous memory), rather than `std::vector`. Providing a `Resources::size()` would more accurately capture this intention anyway. Would you be ok taking that on? - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42751/#review117698 ----------------------------------------------------------- On Feb. 3, 2016, 11:04 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42751/ > ----------------------------------------------------------- > > (Updated Feb. 3, 2016, 11:04 p.m.) > > > Review request for mesos and Michael Park. > > > Repository: mesos > > > Description > ------- > > We should check that two reservations with the same role but different > principals are considered distinct. > > > Diffs > ----- > > src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a > > Diff: https://reviews.apache.org/r/42751/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Neil Conway > >
