You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@mesosphere.io> on 2020/08/14 20:26:55 UTC
Review Request 72745: Added protobuf messages for offer constraints
on a string equality.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72745/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Summary (updated)
-----------------
Added protobuf messages for offer constraints on a string equality.
Bugs: MESOS-10171 and MESOS-10172
https://issues.apache.org/jira/browse/MESOS-10171
https://issues.apache.org/jira/browse/MESOS-10172
Repository: mesos
Description (updated)
-------
Added protobuf messages for offer constraints on a string equality.
Diffs (updated)
-----
include/mesos/scheduler/scheduler.proto 6e1639a9baf017fa87b274daeedc821389216ddc
include/mesos/v1/scheduler/scheduler.proto eb5fdeb984b28403bd8281742bd0a5f2053863e3
Diff: https://reviews.apache.org/r/72745/diff/1/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72745: Added protobuf messages for offer
constraints on a string equality.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Aug. 20, 2020, 7:28 p.m., Benjamin Mahler wrote:
> > include/mesos/scheduler/scheduler.proto
> > Lines 279-285 (original), 279-317 (patched)
> > <https://reviews.apache.org/r/72745/diff/4/?file=2238531#file2238531line279>
> >
> > Ok, after the discussion on slack / reviewboard / and your comment here in the diff, I have a good grasp on the approach and it makes sense.
> >
> > I think we can get away with a simplified naming scheme and logical explanation centered around: we only support text, non text is not supported and will always be matched to let the scheduler do its own filtering.
> >
> > I took a stab at the simplified API naming:
> >
> > ```
> > // Predicates for (pseudo)attribute value equality.
> > //
> > // We currently only support TEXT (string for pseudoattributes)
> > // equality, so these predicates will always match (yield `true`)
> > // for SCALAR or RANGES based attributes. This way, schedulers
> > // will still receive offers for SCALAR and RANGES attributes where
> > // a string equality constraint was specified to mesos, and the
> > // scheduler's own constraint filtering will determine how to
> > // filter these other types of attributes.
> > //
> > // For example: if a scheduler sets a constraint
> > //
> > // { "selector": {"attribute_name": "foo"},
> > // "predicate": {"equals":"2.0"} }
> > //
> > // Agents with a SCALAR attribute {"name": "foo", "scalar": {"value": 1.0}}
> > // will match (since it's SCALAR, not TEXT!) and the scheduler side
> > // filtering will need to filter it according to its desired semantics.
> > //
> > // Therefore, it is strongly recommended to only use TEXT attributes
> > // on agents. For users with existing attributes, this can be done
> > // by adding a variant of the existing attribute that uses TEXT instead:
> > //
> > // --attributes=foo2:v1.0
> > // which gets parsed into
> > // {"name": "foo2", "text": {"value": "v1.0"}}, etc.
> >
> > // Yields `true` if the (pseudo)attribute exists and has a value
> > // equal to this value.
> > //
> > // Non-TEXT (string for pseudoattributes) attributes will always
> > // yield `true` for the reason explained above.
> > message Equals {
> > required string value = 1;
> > }
> >
> > // Yields `true` if the (pseudo)attribute does not exist or has
> > // a value not equal to this value.
> > //
> > // Non-TEXT (string for pseudoattributes) attributes will always
> > // yield `true` for the reason explained above.
> > message NotEquals {
> > required string value = 1;
> > }
> >
> > oneof predicate {
> > Exists exists = 1;
> > NotExists not_exists = 2;
> >
> > Equals equals = 3;
> > NotEquals not_equals = 4;
> > }
> > ```
> >
> > Let me know your thoughts!
>
> Andrei Sekretenko wrote:
> Dropping the "NonStringOr" part definitely makes sense (and a good explanation with examples surely helps).
>
> The only thong I don't like about
> ```
> message Equals {
> required string value;
> }
> ...
> // Java code
> builder.getPredicateBuilder().getEqualsBuilder().setValue("v1.0")
> ```
> is that it does not give any hint to the reader of the code that the case when the attribute is *not even a string* is handled in some special way.
>
> What I'm thinking about is something that would at the very least make the person reading the code wonder what happens to non-string attributes, like
> ```
> message StringEquals {
> required string value;
> }
> ...
> // Java code
> builder.getPredicateBuilder().getStringEqualsBuilder().setValue("v1.0")
> ```
> or, maybe (IMO worse)
> ```
> message Equals {
> required string text;
> }
> ...
> // Java code
> builder.getPredicateBuilder().getEqualsBuilder().setText("v1.0")
> ```
> is that it does not give any hint to the reader of the code that the case when the attribute is not even a string is handled in some special way.
I would hope that seeing that they need to pass in a string prompts thought about non strings (at which point you have to read the comment). Overall I think the "we only support TEXT, and here is how non-TEXT is accomodated" message is pretty clear for a framework developer to reason about.
One thing that I don't like about 'StringEquals' or 'text' is you then need to do:
StringEquals or TextEquals
StringNotEquals or TextNotEquals
StringRegexMatch or TextRegexMatch
StringRegexNonMatch or TextRegexNonMatch
or
required string text
required string textRegex
I think TextEquals/.../TextRegexNonMatch looks most intuitive of these options (and most cleanly supports adding ScalarLessThan etc type of stuff later).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72745/#review221663
-----------------------------------------------------------
On Aug. 20, 2020, 9:09 a.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72745/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2020, 9:09 a.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/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72745: Added protobuf messages for offer
constraints on a string equality.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On Aug. 20, 2020, 7:28 p.m., Benjamin Mahler wrote:
> > include/mesos/scheduler/scheduler.proto
> > Lines 279-285 (original), 279-317 (patched)
> > <https://reviews.apache.org/r/72745/diff/4/?file=2238531#file2238531line279>
> >
> > Ok, after the discussion on slack / reviewboard / and your comment here in the diff, I have a good grasp on the approach and it makes sense.
> >
> > I think we can get away with a simplified naming scheme and logical explanation centered around: we only support text, non text is not supported and will always be matched to let the scheduler do its own filtering.
> >
> > I took a stab at the simplified API naming:
> >
> > ```
> > // Predicates for (pseudo)attribute value equality.
> > //
> > // We currently only support TEXT (string for pseudoattributes)
> > // equality, so these predicates will always match (yield `true`)
> > // for SCALAR or RANGES based attributes. This way, schedulers
> > // will still receive offers for SCALAR and RANGES attributes where
> > // a string equality constraint was specified to mesos, and the
> > // scheduler's own constraint filtering will determine how to
> > // filter these other types of attributes.
> > //
> > // For example: if a scheduler sets a constraint
> > //
> > // { "selector": {"attribute_name": "foo"},
> > // "predicate": {"equals":"2.0"} }
> > //
> > // Agents with a SCALAR attribute {"name": "foo", "scalar": {"value": 1.0}}
> > // will match (since it's SCALAR, not TEXT!) and the scheduler side
> > // filtering will need to filter it according to its desired semantics.
> > //
> > // Therefore, it is strongly recommended to only use TEXT attributes
> > // on agents. For users with existing attributes, this can be done
> > // by adding a variant of the existing attribute that uses TEXT instead:
> > //
> > // --attributes=foo2:v1.0
> > // which gets parsed into
> > // {"name": "foo2", "text": {"value": "v1.0"}}, etc.
> >
> > // Yields `true` if the (pseudo)attribute exists and has a value
> > // equal to this value.
> > //
> > // Non-TEXT (string for pseudoattributes) attributes will always
> > // yield `true` for the reason explained above.
> > message Equals {
> > required string value = 1;
> > }
> >
> > // Yields `true` if the (pseudo)attribute does not exist or has
> > // a value not equal to this value.
> > //
> > // Non-TEXT (string for pseudoattributes) attributes will always
> > // yield `true` for the reason explained above.
> > message NotEquals {
> > required string value = 1;
> > }
> >
> > oneof predicate {
> > Exists exists = 1;
> > NotExists not_exists = 2;
> >
> > Equals equals = 3;
> > NotEquals not_equals = 4;
> > }
> > ```
> >
> > Let me know your thoughts!
>
> Andrei Sekretenko wrote:
> Dropping the "NonStringOr" part definitely makes sense (and a good explanation with examples surely helps).
>
> The only thong I don't like about
> ```
> message Equals {
> required string value;
> }
> ...
> // Java code
> builder.getPredicateBuilder().getEqualsBuilder().setValue("v1.0")
> ```
> is that it does not give any hint to the reader of the code that the case when the attribute is *not even a string* is handled in some special way.
>
> What I'm thinking about is something that would at the very least make the person reading the code wonder what happens to non-string attributes, like
> ```
> message StringEquals {
> required string value;
> }
> ...
> // Java code
> builder.getPredicateBuilder().getStringEqualsBuilder().setValue("v1.0")
> ```
> or, maybe (IMO worse)
> ```
> message Equals {
> required string text;
> }
> ...
> // Java code
> builder.getPredicateBuilder().getEqualsBuilder().setText("v1.0")
> ```
>
> Benjamin Mahler wrote:
> > is that it does not give any hint to the reader of the code that the case when the attribute is not even a string is handled in some special way.
>
> I would hope that seeing that they need to pass in a string prompts thought about non strings (at which point you have to read the comment). Overall I think the "we only support TEXT, and here is how non-TEXT is accomodated" message is pretty clear for a framework developer to reason about.
>
> One thing that I don't like about 'StringEquals' or 'text' is you then need to do:
>
> StringEquals or TextEquals
> StringNotEquals or TextNotEquals
> StringRegexMatch or TextRegexMatch
> StringRegexNonMatch or TextRegexNonMatch
>
> or
>
> required string text
> required string textRegex
>
> I think TextEquals/.../TextRegexNonMatch looks most intuitive of these options (and most cleanly supports adding ScalarLessThan etc type of stuff later).
I've renamed the constraints into TextEquals/TextNotEquals and accommodated the comment you proposed, with small adjustments.
Tried to avoid the phrase "has a value", as this might mislead the reader w.r.t. what happens when there is more than one attribute with the same name.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72745/#review221663
-----------------------------------------------------------
On Aug. 24, 2020, 7:29 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72745/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2020, 7:29 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 9e89c82a7410a6f1d8f62ffff5366673c0fba541
> include/mesos/v1/scheduler/scheduler.proto cd5a980aff7eb820a11c7887e605e50b73425239
>
>
> Diff: https://reviews.apache.org/r/72745/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72745: Added protobuf messages for offer
constraints on a string equality.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On Aug. 20, 2020, 7:28 p.m., Benjamin Mahler wrote:
> > include/mesos/scheduler/scheduler.proto
> > Lines 279-285 (original), 279-317 (patched)
> > <https://reviews.apache.org/r/72745/diff/4/?file=2238531#file2238531line279>
> >
> > Ok, after the discussion on slack / reviewboard / and your comment here in the diff, I have a good grasp on the approach and it makes sense.
> >
> > I think we can get away with a simplified naming scheme and logical explanation centered around: we only support text, non text is not supported and will always be matched to let the scheduler do its own filtering.
> >
> > I took a stab at the simplified API naming:
> >
> > ```
> > // Predicates for (pseudo)attribute value equality.
> > //
> > // We currently only support TEXT (string for pseudoattributes)
> > // equality, so these predicates will always match (yield `true`)
> > // for SCALAR or RANGES based attributes. This way, schedulers
> > // will still receive offers for SCALAR and RANGES attributes where
> > // a string equality constraint was specified to mesos, and the
> > // scheduler's own constraint filtering will determine how to
> > // filter these other types of attributes.
> > //
> > // For example: if a scheduler sets a constraint
> > //
> > // { "selector": {"attribute_name": "foo"},
> > // "predicate": {"equals":"2.0"} }
> > //
> > // Agents with a SCALAR attribute {"name": "foo", "scalar": {"value": 1.0}}
> > // will match (since it's SCALAR, not TEXT!) and the scheduler side
> > // filtering will need to filter it according to its desired semantics.
> > //
> > // Therefore, it is strongly recommended to only use TEXT attributes
> > // on agents. For users with existing attributes, this can be done
> > // by adding a variant of the existing attribute that uses TEXT instead:
> > //
> > // --attributes=foo2:v1.0
> > // which gets parsed into
> > // {"name": "foo2", "text": {"value": "v1.0"}}, etc.
> >
> > // Yields `true` if the (pseudo)attribute exists and has a value
> > // equal to this value.
> > //
> > // Non-TEXT (string for pseudoattributes) attributes will always
> > // yield `true` for the reason explained above.
> > message Equals {
> > required string value = 1;
> > }
> >
> > // Yields `true` if the (pseudo)attribute does not exist or has
> > // a value not equal to this value.
> > //
> > // Non-TEXT (string for pseudoattributes) attributes will always
> > // yield `true` for the reason explained above.
> > message NotEquals {
> > required string value = 1;
> > }
> >
> > oneof predicate {
> > Exists exists = 1;
> > NotExists not_exists = 2;
> >
> > Equals equals = 3;
> > NotEquals not_equals = 4;
> > }
> > ```
> >
> > Let me know your thoughts!
Dropping the "NonStringOr" part definitely makes sense (and a good explanation with examples surely helps).
The only thong I don't like about
```
message Equals {
required string value;
}
...
// Java code
builder.getPredicateBuilder().getEqualsBuilder().setValue("v1.0")
```
is that it does not give any hint to the reader of the code that the case when the attribute is *not even a string* is handled in some special way.
What I'm thinking about is something that would at the very least make the person reading the code wonder what happens to non-string attributes, like
```
message StringEquals {
required string value;
}
...
// Java code
builder.getPredicateBuilder().getStringEqualsBuilder().setValue("v1.0")
```
or, maybe (IMO worse)
```
message Equals {
required string text;
}
...
// Java code
builder.getPredicateBuilder().getEqualsBuilder().setText("v1.0")
```
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72745/#review221663
-----------------------------------------------------------
On Aug. 20, 2020, 9:09 a.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72745/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2020, 9:09 a.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/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72745: Added protobuf messages for offer
constraints on a string equality.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72745/#review221663
-----------------------------------------------------------
include/mesos/scheduler/scheduler.proto
Lines 279-285 (original), 279-317 (patched)
<https://reviews.apache.org/r/72745/#comment310729>
Ok, after the discussion on slack / reviewboard / and your comment here in the diff, I have a good grasp on the approach and it makes sense.
I think we can get away with a simplified naming scheme and logical explanation centered around: we only support text, non text is not supported and will always be matched to let the scheduler do its own filtering.
I took a stab at the simplified API naming:
```
// Predicates for (pseudo)attribute value equality.
//
// We currently only support TEXT (string for pseudoattributes)
// equality, so these predicates will always match (yield `true`)
// for SCALAR or RANGES based attributes. This way, schedulers
// will still receive offers for SCALAR and RANGES attributes where
// a string equality constraint was specified to mesos, and the
// scheduler's own constraint filtering will determine how to
// filter these other types of attributes.
//
// For example: if a scheduler sets a constraint
//
// { "selector": {"attribute_name": "foo"},
// "predicate": {"equals":"2.0"} }
//
// Agents with a SCALAR attribute {"name": "foo", "scalar": {"value": 1.0}}
// will match (since it's SCALAR, not TEXT!) and the scheduler side
// filtering will need to filter it according to its desired semantics.
//
// Therefore, it is strongly recommended to only use TEXT attributes
// on agents. For users with existing attributes, this can be done
// by adding a variant of the existing attribute that uses TEXT instead:
//
// --attributes=foo2:v1.0
// which gets parsed into
// {"name": "foo2", "text": {"value": "v1.0"}}, etc.
// Yields `true` if the (pseudo)attribute exists and has a value
// equal to this value.
//
// Non-TEXT (string for pseudoattributes) attributes will always
// yield `true` for the reason explained above.
message Equals {
required string value = 1;
}
// Yields `true` if the (pseudo)attribute does not exist or has
// a value not equal to this value.
//
// Non-TEXT (string for pseudoattributes) attributes will always
// yield `true` for the reason explained above.
message NotEquals {
required string value = 1;
}
oneof predicate {
Exists exists = 1;
NotExists not_exists = 2;
Equals equals = 3;
NotEquals not_equals = 4;
}
```
Let me know your thoughts!
- Benjamin Mahler
On Aug. 20, 2020, 9:09 a.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72745/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2020, 9:09 a.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/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72745: Added protobuf messages for offer
constraints on a string equality.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72745/#review221692
-----------------------------------------------------------
Ship it!
include/mesos/scheduler/scheduler.proto
Lines 295 (patched)
<https://reviews.apache.org/r/72745/#comment310746>
s/}}}/}} }/
(to be consistent with the space after the opening brace)
include/mesos/scheduler/scheduler.proto
Lines 299 (patched)
<https://reviews.apache.org/r/72745/#comment310745>
maybe no newline here?
- Benjamin Mahler
On Aug. 24, 2020, 7:29 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72745/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2020, 7:29 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 9e89c82a7410a6f1d8f62ffff5366673c0fba541
> include/mesos/v1/scheduler/scheduler.proto cd5a980aff7eb820a11c7887e605e50b73425239
>
>
> Diff: https://reviews.apache.org/r/72745/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72745: Added protobuf messages for offer
constraints on a string equality.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72745/
-----------------------------------------------------------
(Updated Aug. 24, 2020, 7:29 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Renamed into TextEquals/TextNotEquals; extended the comment
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 (updated)
-----
include/mesos/scheduler/scheduler.proto 9e89c82a7410a6f1d8f62ffff5366673c0fba541
include/mesos/v1/scheduler/scheduler.proto cd5a980aff7eb820a11c7887e605e50b73425239
Diff: https://reviews.apache.org/r/72745/diff/5/
Changes: https://reviews.apache.org/r/72745/diff/4-5/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72745: Added protobuf messages for offer
constraints on a string equality.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72745/
-----------------------------------------------------------
(Updated Aug. 20, 2020, 9:09 a.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 (updated)
-----
include/mesos/scheduler/scheduler.proto 6e1639a9baf017fa87b274daeedc821389216ddc
include/mesos/v1/scheduler/scheduler.proto eb5fdeb984b28403bd8281742bd0a5f2053863e3
Diff: https://reviews.apache.org/r/72745/diff/4/
Changes: https://reviews.apache.org/r/72745/diff/3-4/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72745: Added protobuf messages for offer
constraints on a string equality.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Fixed a typo in the comment.
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 (updated)
-----
include/mesos/scheduler/scheduler.proto 6e1639a9baf017fa87b274daeedc821389216ddc
include/mesos/v1/scheduler/scheduler.proto eb5fdeb984b28403bd8281742bd0a5f2053863e3
Diff: https://reviews.apache.org/r/72745/diff/3/
Changes: https://reviews.apache.org/r/72745/diff/2-3/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72745: Added protobuf messages for offer
constraints on a string equality.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> 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
>
>
Re: Review Request 72745: Added protobuf messages for offer
constraints on a string equality.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72745/#review221644
-----------------------------------------------------------
include/mesos/scheduler/scheduler.proto
Lines 275-276 (original), 275-290 (patched)
<https://reviews.apache.org/r/72745/#comment310716>
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?
- Benjamin Mahler
On Aug. 14, 2020, 8:26 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72745/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2020, 8:26 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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>