> On July 13, 2017, 12:35 a.m., Greg Mann wrote: > > One question: if we were to enforce in our validation code that the > > relevant member of `Resource::DiskInfo::Source` must be set when its > > corresponding type is set (in this case, the `path` or `mount` fields), > > would we still want this patch for `operator==`? Should we add that to the > > validation code, or is it a valid state for `type` to be set and the > > corresponding optional member to be unset?
I think that would be another option. It might be more robust to allow `operator==` to return sensible results for _any_ legal `Resource::DiskInfo::Source` object, rather than working only for validated objects (and misbehaving silently for invalid objects); on the other hand, this makes the equality routines a little harder to write. Given that we're already checking for `has_path()` (we're just doing it incorrectly), I'd argue for making this fix, and then potentially revisiting the relationship between validation and equality/etc. routines separately. - Neil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60784/#review180384 ----------------------------------------------------------- On July 11, 2017, 10:29 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60784/ > ----------------------------------------------------------- > > (Updated July 11, 2017, 10:29 p.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > ------- > > The previous implementation was incorrect for the case when either > `right.path` or `right.mount` is set but the corresponding field in > `left` is unset. > > > Diffs > ----- > > src/common/resources.cpp 3f4fff4f1dbe9c8acc55e3077212fe329e53e4d9 > src/v1/resources.cpp 55d493e89114acc94b1524f3f94a47ccea20469a > > > Diff: https://reviews.apache.org/r/60784/diff/1/ > > > Testing > ------- > > `make check` > > I tried to write a unit test for this specific problem but wasn't able to > repro :-\ Current coding seems wrong / inconsistent in any case, though. > > > Thanks, > > Neil Conway > >
