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 16:59:56 UTC
Re: Review Request 72742: Added basic tests for the
`OfferConstraintsFilter`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72742/
-----------------------------------------------------------
(Updated Aug. 14, 2020, 4:59 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Adjusted for discarding of https://reviews.apache.org/r/72740/ and changes in https://reviews.apache.org/r/72741/
Summary (updated)
-----------------
Added basic tests for the `OfferConstraintsFilter`.
Bugs: MESOS-10171
https://issues.apache.org/jira/browse/MESOS-10171
Repository: mesos
Description (updated)
-------
Added basic tests for the `OfferConstraintsFilter`.
Diffs (updated)
-----
src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89
src/tests/CMakeLists.txt cf579f8d6e5176e0d4eadabcbadcbb99aa6c737e
src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/72742/diff/2/
Changes: https://reviews.apache.org/r/72742/diff/1-2/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 72742: Added basic tests for the
`OfferConstraintsFilter`.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On Aug. 18, 2020, 6:36 p.m., Benjamin Mahler wrote:
> > Great to have unit tests for this! Mainly left a suggestion about using a JSON approach to build the protobuf.
> >
> > Do we want to add another test here for attributes with multiple entries? (i.e. showing that we only look at the first entry)
The test for multiple attributes is in https://reviews.apache.org/r/72776, after the equality constraints are implemented; I don't see how to add such a test without a constraint on the value.
JSON looks like a good idea, will try that.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72742/#review221625
-----------------------------------------------------------
On Aug. 14, 2020, 4:59 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72742/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2020, 4:59 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10171
> https://issues.apache.org/jira/browse/MESOS-10171
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added basic tests for the `OfferConstraintsFilter`.
>
>
> Diffs
> -----
>
> src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89
> src/tests/CMakeLists.txt cf579f8d6e5176e0d4eadabcbadcbb99aa6c737e
> src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/72742/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72742: Added basic tests for the
`OfferConstraintsFilter`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72742/#review221625
-----------------------------------------------------------
Great to have unit tests for this! Mainly left a suggestion about using a JSON approach to build the protobuf.
Do we want to add another test here for attributes with multiple entries? (i.e. showing that we only look at the first entry)
src/tests/master/offer_constraints_filter_tests.cpp
Lines 72-76 (patched)
<https://reviews.apache.org/r/72742/#comment310694>
Hm.. see my other comment below, might be able to avoid the `singleAttributeConstraintFilter` by using the same JSON approach to set up the protobuf in all the tests? It would also be nice to have all the OfferConstraintsFilter construction consistent (rather than some going through a helper and some not).
src/tests/master/offer_constraints_filter_tests.cpp
Lines 196-211 (patched)
<https://reviews.apache.org/r/72742/#comment310693>
For this and the others above, it's often pretty hard for a reader to parse through what the protobuf is with setting it up in C++, is it possible to just do a json conversion here as we've done in some other places?
```
Try<JSON::Object> json = JSON::parse<JSON::Object>(
R"~(
{
"role_constraints": {
"role1": {
"attribute_constraints": [{
"selector": {"attribute_name": "foo"},
"predicate": {"exists": {}}
}]
},
"role2": {
"attribute_constraints": [{
"selector": {"attribute_name": "bar"},
"predicate": {"not_exists": {}}
}]
}
}
})~");
ASSERT_SOME(json);
Try<OfferConstraints> constraints = protobuf::parse(*json);
ASSERT_SOME(constraints);
```
This makes it really easy on the reader to see what the constraints look like.
- Benjamin Mahler
On Aug. 14, 2020, 4:59 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72742/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2020, 4:59 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10171
> https://issues.apache.org/jira/browse/MESOS-10171
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added basic tests for the `OfferConstraintsFilter`.
>
>
> Diffs
> -----
>
> src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89
> src/tests/CMakeLists.txt cf579f8d6e5176e0d4eadabcbadcbb99aa6c737e
> src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/72742/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72742: Added basic tests for the
`OfferConstraintsFilter`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72742/#review221662
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Mahler
On Aug. 19, 2020, noon, Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72742/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2020, noon)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10171
> https://issues.apache.org/jira/browse/MESOS-10171
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added basic tests for the `OfferConstraintsFilter`.
>
>
> Diffs
> -----
>
> src/Makefile.am 6d68ed050f99889c142d49bbc72a9292ef64c836
> src/tests/CMakeLists.txt cf579f8d6e5176e0d4eadabcbadcbb99aa6c737e
> src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/72742/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72742: Added basic tests for the
`OfferConstraintsFilter`.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72742/
-----------------------------------------------------------
(Updated Aug. 19, 2020, noon)
Review request for mesos and Benjamin Mahler.
Changes
-------
Switched to JSON definitions of constraints.
Bugs: MESOS-10171
https://issues.apache.org/jira/browse/MESOS-10171
Repository: mesos
Description
-------
Added basic tests for the `OfferConstraintsFilter`.
Diffs (updated)
-----
src/Makefile.am 6d68ed050f99889c142d49bbc72a9292ef64c836
src/tests/CMakeLists.txt cf579f8d6e5176e0d4eadabcbadcbb99aa6c737e
src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/72742/diff/4/
Changes: https://reviews.apache.org/r/72742/diff/3-4/
Testing
-------
Thanks,
Andrei Sekretenko