> On Aug. 19, 2020, 7:03 p.m., Benjamin Mahler wrote: > > include/mesos/scheduler/scheduler.proto > > Lines 275-276 (original), 275-290 (patched) > > <https://reviews.apache.org/r/72745/diff/2/?file=2238095#file2238095line275> > > > > I'm still struggling to get a good grasp on the cases here based on the > > comments. > > > > Why does the first one say "not equal"? > > > > The following seems more towards how I would assume these work: > > > > ``` > > // If the (pseudo)attribute exists and is a string or TEXT, returns > > // value == (pseudo)attribute value. Otherwise, returns false: > > // > > // (pseudo)attribute exists && > > // (pseudo)attribute type is string or TEXT && > > // (pseudo)attribute value == value > > // > > message StringEquals { ... } > > > > // If the (pseudo)attribute exists and is a string or TEXT, returns > > // value != (pseudo)attribute value. Otherwise, returns true: > > // > > // !(pseudo)attribute exists || > > // !(pseudo)attribute type is string or TEXT || > > // (pseudo)attribute value != value > > // > > message StringNotEquals { ... } > > ``` > > > > But, NonStringOrEqualsString is doing: > > > > ``` > > // (pseudo)attribute exists && ( > > // !(pseudo)attribute type is string or TEXT || > > // (pseudo)attribute value == value > > // ) > > ``` > > > > Seems weird? if an agent has "foo: 1" and the constraint is "foo: abc", > > we'll offer that agent?
Sorry, that was a typo in the comment. Thanks for catching that! The code and tests implement the intended behaviour; the comment for `NonStringOrEqualsString` saying that `not equal` was wrong. What should be the intended behavoiur for the text equality constraint for non-TEXT attributes, is actually a very good question. The problem with NOT offering an agent with `foo:1` in attributes if there is a constraint on a string equality (`"foo":"abc"`, as in your example, or `"foo": "1"`)is that it is not consistent with the regexp match constraint in the further patches (`NonStringOrMatchesRegex`). The latter simply **has** to pass all non-string attributes, so that frameworks can decide on their own what does it mean for an agent with `foo:1` to match `"foo":"1.0.*"` - for example, Marathon treats this as a match currently. If you think that local sanity is more important than consistency with regex match, let's change this to `EqualsString` which will yield `false` for non-TEXT attributes. Honestly, I don't like any of these two options ;) - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72745/#review221644 ----------------------------------------------------------- On Aug. 19, 2020, 8:16 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72745/ > ----------------------------------------------------------- > > (Updated Aug. 19, 2020, 8:16 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10172 > https://issues.apache.org/jira/browse/MESOS-10172 > > > Repository: mesos > > > Description > ------- > > This patch adds protobuf messages for setting offer constraints > on equality/non-equality of agent's (pseudo)attribute to a specified > string. > > Both added contsraint predicates will evaluate to `true` when the > attribute is not TEXT. This way, schedulers will still perform all > filtration based on non-TEXT attributes on their own, which helps to > avoid subtle integration bugs caused by discrepancies in Scalar/Ranges > comparison between Mesos and the scheduler. > > Given that schedulers seem to rarely put constraints on Scalar/Ranges > attributes in the real world, this should not prevent them from > obtaining performance benefits by setting offer constraints. > > > Diffs > ----- > > include/mesos/scheduler/scheduler.proto > 6e1639a9baf017fa87b274daeedc821389216ddc > include/mesos/v1/scheduler/scheduler.proto > eb5fdeb984b28403bd8281742bd0a5f2053863e3 > > > Diff: https://reviews.apache.org/r/72745/diff/3/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
