Re: Review Request 67238: Fixed a quota-related metrics bug.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67238/#review203575 --- Ship it! Ship It! - Gaston Kleiman On May 21, 2018, 3:13 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67238/ > --- > > (Updated May 21, 2018, 3:13 p.m.) > > > Review request for mesos, Benjamin Mahler, Gaston Kleiman, Gilbert Song, and > Vinod Kone. > > > Bugs: MESOS-8932 > https://issues.apache.org/jira/browse/MESOS-8932 > > > Repository: mesos > > > Description > --- > > Fixed a quota-related metrics bug. > > > Diffs > - > > src/master/allocator/mesos/metrics.cpp > 5194f5b3b3f507b36f02300775230157db3dd477 > src/tests/master_quota_tests.cpp ddb2903c0b60f2929e39d6aa23dc924aec454c69 > > > Diff: https://reviews.apache.org/r/67238/diff/1/ > > > Testing > --- > > `make check` passes > > Also, the modified test in this patch, `MasterQuotaTest.RemoveSingleQuota`, > fails without the metrics fix and passes with it. > > > Thanks, > > Greg Mann > >
Re: Review Request 66883: Added/updated tests to check per framework metrics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66883/ --- (Updated May 14, 2018, 5:19 p.m.) Review request for Benjamin Mahler, Gilbert Song, Greg Mann, and Vinod Kone. Repository: mesos Description --- Added/updated tests to check per framework metrics. Diffs - src/tests/master_tests.cpp d5ce52c0eb1ba333d1b3dd35518545c13d828ce6 Diff: https://reviews.apache.org/r/66883/diff/1/ Testing --- Thanks, Gaston Kleiman
Re: Review Request 66882: Added `normalizeMetricKey()` and `getFrameworkMetricPrefix()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66882/ --- (Updated May 14, 2018, 4:47 p.m.) Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, Jie Yu, and Vinod Kone. Repository: mesos Description --- Added `normalizeMetricKey()` and `getFrameworkMetricPrefix()`. Diffs (updated) - src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 Diff: https://reviews.apache.org/r/66882/diff/4/ Changes: https://reviews.apache.org/r/66882/diff/3-4/ Testing --- Thanks, Gaston Kleiman
Re: Review Request 66882: Added `normalizeMetricKey()` and `getFrameworkMetricPrefix()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66882/ --- (Updated May 9, 2018, 12:35 p.m.) Review request for mesos, Benjamin Mahler, Gilbert Song, Greg Mann, and Vinod Kone. Changes --- Added extra blank lines. Repository: mesos Description --- Added `normalizeMetricKey()` and `getFrameworkMetricPrefix()`. Diffs (updated) - src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 Diff: https://reviews.apache.org/r/66882/diff/3/ Changes: https://reviews.apache.org/r/66882/diff/2-3/ Testing --- Thanks, Gaston Kleiman
Re: Review Request 66822: Added per Framework Calls to metrics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66822/#review202778 --- src/master/metrics.hpp Lines 239 (patched) <https://reviews.apache.org/r/66822/#comment284775> Thsi map should be named `call_types` for consistency with all the maps in `Master::Metrics`. - Gaston Kleiman On April 30, 2018, 11:45 a.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66822/ > --- > > (Updated April 30, 2018, 11:45 a.m.) > > > Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, and > Vinod Kone. > > > Repository: mesos > > > Description > --- > > Added per Framework Calls to metrics. > > > Diffs > - > > src/master/http.cpp 135ae4389623385a6638158f5f23d6daca14a0ad > src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 > src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b > src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 > > > Diff: https://reviews.apache.org/r/66822/diff/4/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 66882: Added `normalizeMetricKey()` and `getFrameworkMetricPrefix()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66882/ --- (Updated May 8, 2018, 5:06 p.m.) Review request for mesos, Benjamin Mahler, Gilbert Song, Greg Mann, and Vinod Kone. Summary (updated) - Added `normalizeMetricKey()` and `getFrameworkMetricPrefix()`. Repository: mesos Description (updated) --- Added `normalizeMetricKey()` and `getFrameworkMetricPrefix()`. Diffs (updated) - src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 Diff: https://reviews.apache.org/r/66882/diff/2/ Changes: https://reviews.apache.org/r/66882/diff/1-2/ Testing --- Thanks, Gaston Kleiman
Re: Review Request 66822: Added per Framework Calls to metrics.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66822/#review202680
---
src/master/metrics.cpp
Lines 556-557 (patched)
<https://reviews.apache.org/r/66822/#comment284653>
Fits in one line.
src/master/metrics.cpp
Lines 595-623 (patched)
<https://reviews.apache.org/r/66822/#comment284657>
We could replace these two methods with a single method that that takes a
`const scheduler::Call::Type&` instead of a `const scheduler::Call&`.
We use this pattern for a lot of metrics, so should consider adding
something like:
```
template
class EnumCounter
{
EnumCounter(const lambda::function<std::string(const Type&)>
_keyNameGenerator);
~EnumCounter();
void increment(const Type& type);
private:
hashmap<Type, Counter> counters;
const lambda::function<std::string(const Type&)> keyNameGenerator;
}
```
- Gaston Kleiman
On April 30, 2018, 11:45 a.m., Gilbert Song wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66822/
> ---
>
> (Updated April 30, 2018, 11:45 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, and
> Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Added per Framework Calls to metrics.
>
>
> Diffs
> -
>
> src/master/http.cpp 135ae4389623385a6638158f5f23d6daca14a0ad
> src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47
> src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b
> src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45
>
>
> Diff: https://reviews.apache.org/r/66822/diff/4/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 66821: Added framwork metrics helper normalize() and getPrefix().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66821/#review202663 --- There's a typo in the RR description: s/framwork/framework/ src/master/metrics.hpp Lines 226-227 (patched) <https://reviews.apache.org/r/66821/#comment284602> This is done in https://reviews.apache.org/r/66882/, can we squash both patches? - Gaston Kleiman On April 30, 2018, 11:41 a.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66821/ > --- > > (Updated April 30, 2018, 11:41 a.m.) > > > Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, and > Vinod Kone. > > > Repository: mesos > > > Description > --- > > Added framwork metrics helper normalize() and getPrefix(). > > > Diffs > - > > src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b > src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 > > > Diff: https://reviews.apache.org/r/66821/diff/3/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 66819: Added FrameworkMetrics struct in framework struct.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66819/#review202662 --- src/master/metrics.hpp Lines 219-221 (patched) <https://reviews.apache.org/r/66819/#comment284601> This constructor should be implemented in this patch. The implementation is introduced in https://reviews.apache.org/r/66820/ with extra parameters. - Gaston Kleiman On April 27, 2018, 11:39 a.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66819/ > --- > > (Updated April 27, 2018, 11:39 a.m.) > > > Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, and > Vinod Kone. > > > Repository: mesos > > > Description > --- > > Added FrameworkMetrics struct in framework struct. > > > Diffs > - > > src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 > src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 > > > Diff: https://reviews.apache.org/r/66819/diff/3/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Review Request 66995: Prevented Master::drop() from sending operation updates to v0 framework.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66995/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- Prevented master::drop() from sending operation updates to v0 framework. Testing --- `sudo bin/mesos-tests.sh` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66992: Made the master drop operations with an ID on non-default resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66992/ --- (Updated May 7, 2018, 5:04 p.m.) Review request for mesos and Greg Mann. Changes --- Addressed feedback. Repository: mesos Description --- Made the master drop operations with an ID on non-default resources. Diffs (updated) - src/master/master.cpp 3b5d2eba3f602f68a6bb1e00444b01fb58a1bfc2 src/tests/master_tests.cpp e159573b550b07f5315878a6eabb3b84080bf15a src/tests/operation_reconciliation_tests.cpp 9717e8454a7d4654c58b903a8420aee2b0cea8a2 Diff: https://reviews.apache.org/r/66992/diff/2/ Changes: https://reviews.apache.org/r/66992/diff/1-2/ Testing --- `sudo bin/mesos-tests.sh` on GNU/Linux Thanks, Gaston Kleiman
Review Request 66992: Made the master drop operations with an ID on non-default resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66992/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- Made the master drop operations with an ID on non-default resources. Diffs - src/master/master.cpp 3b5d2eba3f602f68a6bb1e00444b01fb58a1bfc2 src/tests/master_tests.cpp e159573b550b07f5315878a6eabb3b84080bf15a src/tests/operation_reconciliation_tests.cpp 9717e8454a7d4654c58b903a8420aee2b0cea8a2 Diff: https://reviews.apache.org/r/66992/diff/1/ Testing --- `sudo bin/mesos-tests.sh` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66924: Made the master include the operation ID in OPERATION_DROPPED updates.
> On May 7, 2018, 9:41 a.m., Greg Mann wrote: > > src/tests/master_slave_reconciliation_tests.cpp > > Lines 496 (patched) > > <https://reviews.apache.org/r/66924/diff/2/?file=2017096#file2017096line496> > > > > Nit: indented too far. I ended up removing this callback, as it makes the test flaky and isn't really useful. > On May 7, 2018, 9:41 a.m., Greg Mann wrote: > > src/tests/mesos.hpp > > Lines 469 (patched) > > <https://reviews.apache.org/r/66924/diff/2/?file=2017097#file2017097line469> > > > > Is this necessary? Yes, it is necessary in order to use `v1::UUID` in the test instead of `mesos::v1::UUID`. - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66924/#review202555 ------- On May 7, 2018, 11:54 a.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66924/ > --- > > (Updated May 7, 2018, 11:54 a.m.) > > > Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Greg Mann. > > > Bugs: MESOS-8784 > https://issues.apache.org/jira/browse/MESOS-8784 > > > Repository: mesos > > > Description > --- > > Made the master include the operation ID in OPERATION_DROPPED updates. > > > Diffs > - > > src/master/master.hpp 76e7763a99972f686af9d7eed08b6b6b1db23b0f > src/master/master.cpp 3b5d2eba3f602f68a6bb1e00444b01fb58a1bfc2 > src/tests/master_slave_reconciliation_tests.cpp > 71e22af7ac3b7bc4c72340274961db16d7355e7d > src/tests/mesos.hpp 8da3b021874097c5b66e5bc7d9fdafcc8fc377ef > > > Diff: https://reviews.apache.org/r/66924/diff/3/ > > > Testing > --- > > `make check` on GNU/Linux. > > > The test in this patch fails without the corresponding master change, but > succeeds with it applied. > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66924: Made the master include the operation ID in OPERATION_DROPPED updates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66924/ --- (Updated May 7, 2018, 11:54 a.m.) Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Greg Mann. Changes --- Addressed feedback. Bugs: MESOS-8784 https://issues.apache.org/jira/browse/MESOS-8784 Repository: mesos Description --- Made the master include the operation ID in OPERATION_DROPPED updates. Diffs (updated) - src/master/master.hpp 76e7763a99972f686af9d7eed08b6b6b1db23b0f src/master/master.cpp 3b5d2eba3f602f68a6bb1e00444b01fb58a1bfc2 src/tests/master_slave_reconciliation_tests.cpp 71e22af7ac3b7bc4c72340274961db16d7355e7d src/tests/mesos.hpp 8da3b021874097c5b66e5bc7d9fdafcc8fc377ef Diff: https://reviews.apache.org/r/66924/diff/3/ Changes: https://reviews.apache.org/r/66924/diff/2-3/ Testing --- `make check` on GNU/Linux. The test in this patch fails without the corresponding master change, but succeeds with it applied. Thanks, Gaston Kleiman
Re: Review Request 66939: Improved validation messages for some operations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66939/ --- (Updated May 4, 2018, 4:41 p.m.) Review request for mesos, Greg Mann and Jan Schlicht. Changes --- Fixed tests. Repository: mesos Description --- Improved validation messages for some operations. Diffs (updated) - src/master/validation.cpp 0c1c9249a0c1ca5ca52e5d7f32a1f02f6e2078d1 src/tests/master_validation_tests.cpp fb1d8bdf9ba09cace3ceb202dc70c0d201f2784e Diff: https://reviews.apache.org/r/66939/diff/3/ Changes: https://reviews.apache.org/r/66939/diff/2-3/ Testing --- Thanks, Gaston Kleiman
Re: Review Request 66924: Made the master include the operation ID in OPERATION_DROPPED updates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66924/ --- (Updated May 4, 2018, 4:37 p.m.) Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Greg Mann. Changes --- Addressed feedback. Bugs: MESOS-8784 https://issues.apache.org/jira/browse/MESOS-8784 Repository: mesos Description --- Made the master include the operation ID in OPERATION_DROPPED updates. Diffs (updated) - src/master/master.hpp 76e7763a99972f686af9d7eed08b6b6b1db23b0f src/master/master.cpp 3b5d2eba3f602f68a6bb1e00444b01fb58a1bfc2 src/tests/master_slave_reconciliation_tests.cpp 71e22af7ac3b7bc4c72340274961db16d7355e7d src/tests/mesos.hpp 8da3b021874097c5b66e5bc7d9fdafcc8fc377ef Diff: https://reviews.apache.org/r/66924/diff/2/ Changes: https://reviews.apache.org/r/66924/diff/1-2/ Testing --- `make check` on GNU/Linux. The test in this patch fails without the corresponding master change, but succeeds with it applied. Thanks, Gaston Kleiman
Re: Review Request 66965: Added MESOS-8054 to the 1.6 CHANGELOG.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66965/#review202494 --- Ship it! Ship It! - Gaston Kleiman On May 4, 2018, 3:35 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66965/ > --- > > (Updated May 4, 2018, 3:35 p.m.) > > > Review request for mesos and Gaston Kleiman. > > > Repository: mesos > > > Description > --- > > Added MESOS-8054 to the 1.6 CHANGELOG. > > > Diffs > - > > CHANGELOG 02c7d6f82e0bb24c23143f696f29c57076456fa0 > > > Diff: https://reviews.apache.org/r/66965/diff/2/ > > > Testing > --- > > > Thanks, > > Greg Mann > >
Re: Review Request 66924: Made the master include the operation ID in OPERATION_DROPPED updates.
> On May 4, 2018, 5:39 a.m., Benjamin Bannier wrote: > > src/master/master.hpp > > Line 442 (original), 442 (patched) > > <https://reviews.apache.org/r/66924/diff/1/?file=2016644#file2016644line442> > > > > The original pattern here seems to have been to always consitently > > break after the opening paren. > > > > Let's not change that. We seem to only break after the opening paren if the first parameter is a `const process::UPID&`, see these cases in which we don't insert a line break: https://github.com/apache/mesos/blob/351bade6c28682daf821e88a40140e1364d69cb0/src/master/master.hpp#L523 https://github.com/apache/mesos/blob/351bade6c28682daf821e88a40140e1364d69cb0/src/master/master.hpp#L546 https://github.com/apache/mesos/blob/351bade6c28682daf821e88a40140e1364d69cb0/src/master/master.hpp#L569 https://github.com/apache/mesos/blob/351bade6c28682daf821e88a40140e1364d69cb0/src/master/master.hpp#L573 > On May 4, 2018, 5:39 a.m., Benjamin Bannier wrote: > > src/tests/storage_local_resource_provider_tests.cpp > > Lines 3500 (patched) > > <https://reviews.apache.org/r/66924/diff/1/?file=2016646#file2016646line3500> > > > > The behavior described in the comment above seems rather simple, but we > > use a pretty complicated setup below. This not only leads to us writing too > > much code unrelated to the thing under test, but also introduces unneeded > > dependencies on behavior we don't care about (here) -- e.g., I don't think > > this test depends on a SLRP. I would also much prefer a non-`ROOT` test so > > we run this as often as possible (I suspect developers do not regularly run > > `make check` as root on their development machines). > > > > I would suggest to reimplement the test with a `MockResourceProvider` > > if we do really need a `CREATE_VOLUME` operation below -- but it seems we > > should be able to perform this test with even non-RP operations which would > > simplify things further. > > > > See e.g., the test added in r/66908 which needs 60% less test code. I had originally implemented test like yours in r/66908, but then remembered that Mesos 1.6.0 will not support operation feedback on non-RP resources; I am going to add a validation rejecting operations that don't affect RP resources but have an operation ID, so we need a test that uses a resource provider. I agree that there is too much boiler plate in the test, I also think we should make it easier to write tests using a SLRP. We currently have no test that checks that the master correctly forwards the `OPERATION_DROPPED` updates generated by a SLRP when an operation ID is specified. This test checks that, but if I reimplement it to use a `MockResourceProvider`, then we should check in `StorageLocalResourceProviderTest.ROOT_ReconcileDroppedOperation` whether the update is correctly forwarded to the scheduler. - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66924/#review202436 --- On May 3, 2018, 5 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66924/ > --- > > (Updated May 3, 2018, 5 p.m.) > > > Review request for mesos, Benjamin Bannier and Greg Mann. > > > Bugs: MESOS-8784 > https://issues.apache.org/jira/browse/MESOS-8784 > > > Repository: mesos > > > Description > --- > > Made the master include the operation ID in OPERATION_DROPPED updates. > > > Diffs > - > > src/master/master.hpp c30cf082d9ddf2e5dd97e0d887d6d07d8e03cbd4 > src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 > src/tests/storage_local_resource_provider_tests.cpp > ccb114aacc904998dfeef4128f6d38dfb3c865c7 > > > Diff: https://reviews.apache.org/r/66924/diff/1/ > > > Testing > --- > > `make check` on GNU/Linux. > > > The test in this patch fails without the corresponding master change, but > succeeds with it applied. > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66938: Made the 'SchedulerDriver' abort when operation's 'id' field is set.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66938/#review202413 --- Ship it! Ship It! - Gaston Kleiman On May 3, 2018, 4:14 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66938/ > --- > > (Updated May 3, 2018, 4:14 p.m.) > > > Review request for mesos, Benjamin Bannier, Gaston Kleiman, Jan Schlicht, and > Vinod Kone. > > > Repository: mesos > > > Description > --- > > Since the 'SchedulerDriver' does not support operation status updates, > this patch adds a check to the driver which will abort the scheduler > if the 'id' field is set in an offer operation. > > > Diffs > - > > src/sched/sched.cpp 620a3b26d8bf3487b6ce922b2280be7da291df06 > > > Diff: https://reviews.apache.org/r/66938/diff/2/ > > > Testing > --- > > make check > > > Thanks, > > Greg Mann > >
Review Request 66924: Made the master include the operation ID in OPERATION_DROPPED updates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66924/ --- Review request for mesos, Benjamin Bannier and Greg Mann. Bugs: MESOS-8784 https://issues.apache.org/jira/browse/MESOS-8784 Repository: mesos Description --- Made the master include the operation ID in OPERATION_DROPPED updates. Diffs - src/master/master.hpp c30cf082d9ddf2e5dd97e0d887d6d07d8e03cbd4 src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 src/tests/storage_local_resource_provider_tests.cpp ccb114aacc904998dfeef4128f6d38dfb3c865c7 Diff: https://reviews.apache.org/r/66924/diff/1/ Testing --- `make check` on GNU/Linux. The test in this patch fails without the corresponding master change, but succeeds with it applied. Thanks, Gaston Kleiman
Re: Review Request 66939: Improved validation messages for some operations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66939/ --- (Updated May 3, 2018, 4:54 p.m.) Review request for mesos, Greg Mann and Jan Schlicht. Changes --- Addressed Greg's feedback. I guess I need new glasses ¯\_(?)_/¯. Repository: mesos Description --- Improved validation messages for some operations. Diffs (updated) - src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d Diff: https://reviews.apache.org/r/66939/diff/2/ Changes: https://reviews.apache.org/r/66939/diff/1-2/ Testing --- Thanks, Gaston Kleiman
Review Request 66939: Improved validation messages for some operations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66939/ --- Review request for mesos, Greg Mann and Jan Schlicht. Repository: mesos Description --- Improved validation messages for some operations. Diffs - src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d Diff: https://reviews.apache.org/r/66939/diff/1/ Testing --- Thanks, Gaston Kleiman
Re: Review Request 66938: Made the 'SchedulerDriver' abort when operation's 'id' field is set.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66938/#review202388 --- src/sched/sched.cpp Lines 1349-1352 (patched) <https://reviews.apache.org/r/66938/#comment284199> We should call `error()` here so that the framework is gracefully teared down instead of triggering a segfault. - Gaston Kleiman On May 3, 2018, 3:07 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66938/ > --- > > (Updated May 3, 2018, 3:07 p.m.) > > > Review request for mesos, Benjamin Bannier, Gaston Kleiman, Jan Schlicht, and > Vinod Kone. > > > Repository: mesos > > > Description > --- > > Since the 'SchedulerDriver' does not support operation status updates, > this patch adds a CHECK to the driver which will abort the scheduler > if the 'id' field is set in an offer operation. > > This patch also removes the test > 'SchedulerTest.OperationFeedbackValidationSchedulerDriverFramework' > because with these new changes, it causes the test harness to abort. > > > Diffs > - > > src/sched/sched.cpp 620a3b26d8bf3487b6ce922b2280be7da291df06 > src/tests/scheduler_tests.cpp 749420a23bc1a3873bd4d5aee56e78cff79bb1af > > > Diff: https://reviews.apache.org/r/66938/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Greg Mann > >
Re: Review Request 66849: Added missing test expectation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66849/#review202367 --- Ship it! Ship It! - Gaston Kleiman On April 27, 2018, 3:15 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66849/ > --- > > (Updated April 27, 2018, 3:15 a.m.) > > > Review request for mesos, Gaston Kleiman and Greg Mann. > > > Repository: mesos > > > Description > --- > > On master failover the scheduler will get disconnected. Add a test > expectation for that. This silences a gmock warning. > > > Diffs > - > > src/tests/operation_reconciliation_tests.cpp > 76c169567de6e0ef99c984a673cd69ce7725d6b6 > > > Diff: https://reviews.apache.org/r/66849/diff/1/ > > > Testing > --- > > `make check` > > Before this patch the fixed tests emits a gmock warning about an unexpected > function call, with this patch the warning disappears. > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 66908: Correctly reconciled dropped operation after agent failover.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66908/#review202323 --- Fix it, then Ship it! src/tests/master_slave_reconciliation_tests.cpp Lines 371-372 (patched) <https://reviews.apache.org/r/66908/#comment284084> Nit: many of the tests we already have do this instead: ``` FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO; frameworkInfo.set_roles(0, DEFAULT_TEST_ROLE); ``` - Gaston Kleiman On May 2, 2018, 7:35 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66908/ > --- > > (Updated May 2, 2018, 7:35 a.m.) > > > Review request for mesos, Gaston Kleiman and Greg Mann. > > > Bugs: MESOS-8870 > https://issues.apache.org/jira/browse/MESOS-8870 > > > Repository: mesos > > > Description > --- > > When the master receives an `UpdateSlaveMessage` after agent failover > it previously did not correctly detect dropped operations (operations > known to the master, but unknown to the agent) and did not trigger > reconciliation for such operations. > > This patch fixes the handler in the master so that such dropped > operations are reconciled. > > > Diffs > - > > src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 > src/tests/master_slave_reconciliation_tests.cpp > 6bb4263323bcfd191c8e3c1ccba10a240e9ddd83 > > > Diff: https://reviews.apache.org/r/66908/diff/1/ > > > Testing > --- > > `make check` > > The test in this patch fails without the corresponding master change, but > succeeds with it applied. > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/ --- (Updated April 23, 2018, 1:23 p.m.) Review request for mesos, Greg Mann and Vinod Kone. Changes --- Addressed feedback. Bugs: MESOS-8192 https://issues.apache.org/jira/browse/MESOS-8192 Repository: mesos Description --- This patch adds a `call()` method to the scheduler library that allows clients to send a `v1::scheduler::Call` to the master and receive a `v1::scheduler::Response`. It is going to be used to test operation state reconciliation. Diffs (updated) - include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 include/mesos/v1/scheduler/scheduler.proto 25ebcfcb07cb4fb928dcfdc242bb6509c9080491 src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 60b17b9be74132c81532d22eba681feb899b22a3 src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 Diff: https://reviews.apache.org/r/66460/diff/5/ Changes: https://reviews.apache.org/r/66460/diff/4-5/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66644: Remove unknown unreachable tasks when agent re-registers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66644/#review201750 --- src/master/master.cpp Lines 7130 (patched) <https://reviews.apache.org/r/66644/#comment283338> I'd add a comment here explaining what we are doing. Maybe something like: "All tasks from this agent are now reachable. Update framework bookkeeping, removing tasks that were dropped en-route to the agent and marked as unreachable." src/tests/partition_tests.cpp Lines 167-168 (patched) <https://reviews.apache.org/r/66644/#comment283324> I'd write something like: ``` This is a regression test for MESOS-8750. It verifies that a master won't crash when a `RunTaskMessage` associated to a partition aware framework is dropped en route to the agent and then the agent fails over. ``` src/tests/partition_tests.cpp Lines 169 (patched) <https://reviews.apache.org/r/66644/#comment283308> s/cleanUpUnreachableTasks/CleanUpUnreachableTasks/ src/tests/partition_tests.cpp Lines 182 (patched) <https://reviews.apache.org/r/66644/#comment283309> The indentation here looks off. src/tests/partition_tests.cpp Lines 200 (patched) <https://reviews.apache.org/r/66644/#comment283312> Ditto indentation. src/tests/partition_tests.cpp Lines 206-207 (patched) <https://reviews.apache.org/r/66644/#comment283313> Ditto indentation. src/tests/partition_tests.cpp Lines 216 (patched) <https://reviews.apache.org/r/66644/#comment283317> s/Offer/const Offer&/ src/tests/partition_tests.cpp Lines 220 (patched) <https://reviews.apache.org/r/66644/#comment283320> s/Capture/Drop/ ? Where is the pid unset? I think I'd write something like: "Drop the `RunTaskMessage`", so that the agent doesn't learn about the new task. src/tests/partition_tests.cpp Lines 222 (patched) <https://reviews.apache.org/r/66644/#comment283318> Ditto indentation. src/tests/partition_tests.cpp Lines 228 (patched) <https://reviews.apache.org/r/66644/#comment283319> Remove this blank line. src/tests/partition_tests.cpp Lines 235 (patched) <https://reviews.apache.org/r/66644/#comment283323> Ditto indentation. src/tests/partition_tests.cpp Lines 241 (patched) <https://reviews.apache.org/r/66644/#comment283325> Ditto indentation. src/tests/partition_tests.cpp Lines 279-282 (patched) <https://reviews.apache.org/r/66644/#comment283326> Ditto indentation. src/tests/partition_tests.cpp Lines 303 (patched) <https://reviews.apache.org/r/66644/#comment283327> Ditto indentation. src/tests/partition_tests.cpp Lines 317 (patched) <https://reviews.apache.org/r/66644/#comment283328> Ditto indentation. src/tests/partition_tests.cpp Lines 319-323 (patched) <https://reviews.apache.org/r/66644/#comment283329> Ditto indentation. src/tests/partition_tests.cpp Lines 328-332 (patched) <https://reviews.apache.org/r/66644/#comment283330> Ditto indentation. src/tests/partition_tests.cpp Lines 346-350 (patched) <https://reviews.apache.org/r/66644/#comment283331> Ditto indentation. src/tests/partition_tests.cpp Lines 355-359 (patched) <https://reviews.apache.org/r/66644/#comment283332> Ditto indentation. src/tests/partition_tests.cpp Lines 379 (patched) <https://reviews.apache.org/r/66644/#comment283336> s/reregister/re-register/ src/tests/partition_tests.cpp Lines 384 (patched) <https://reviews.apache.org/r/66644/#comment28> Ditto indentation. src/tests/partition_tests.cpp Lines 433-437 (patched) <https://reviews.apache.org/r/66644/#comment283334> Ditto indentation. src/tests/partition_tests.cpp Lines 452-456 (patched) <https://reviews.apache.org/r/66644/#comment283335> Ditto indentation. - Gaston Kleiman On April 23, 2018, 11:11 a.m., Megha Sharma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66644/ > --- > > (Updated April 23, 2018, 11:11 a.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: 8750 > https://issues.apache.org/jira/browse/8750 > > > Repository: mesos > > > Description > --- > > A RunTask messsage could get dropped for an agent while it's > disconnected from the master and when such an agent goes unreachable > then this dropped task message gets added to the unreachable tasks. > When the agent re-registers, the master sends status updates for the > tasks that the agent reporte
Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review201673
---
Changed the return type to the following proto message:
```
/**
* This message is used ty the C++ Scheduler HTTP API library as the return
* type of the `call()` method. The message includes the HTTP status code with
* which the master responded, and optionally a `scheduler::Response` message.
*
* There are three cases to consider depending on the HTTP response status code:
*
* (1) '202 ACCEPTED': Indicates the call was accepted for processing and
*neither `response` nor `error` will be set.
*
* (2) '200 OK': Indicates the call completed successfully, and the `response`
*field will be set if the `scheduler::Call::Type` has a corresponding
*`scheduler::Response::Type`; `error` will not be set.
*
* (3) For all other HTTP status codes, the `response` field will not be set
* and the `error` field may be set to provide more information.
*
* NOTE: This message is used by the C++ Scheduler HTTP API library and not part
* of the API specification.
*/
message APIResult {
// HTTP status code with which the master responded.
required uint32 status_code = 1;
// This field will only be set if the call completed successfully and the
// master responded with `200 OK` and a non-empty body.
optional Response response = 2;
// This field will only be set if the call did not complete successfully and
// the master responded with a status other than `202 Accepted` or `200 OK`,
// and with a non-empty body.
optional string error = 3;
}
```
I tried to reply to BenM's issue, but Review Board doesn't let me publish the
draft ¯_(?)_/¯.
- Gaston Kleiman
On April 20, 2018, 5:03 p.m., Gaston Kleiman wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66460/
> ---
>
> (Updated April 20, 2018, 5:03 p.m.)
>
>
> Review request for mesos, Greg Mann and Vinod Kone.
>
>
> Bugs: MESOS-8192
> https://issues.apache.org/jira/browse/MESOS-8192
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This patch adds a `call()` method to the scheduler library that allows
> clients to send a `v1::scheduler::Call` to the master and receive a
> `v1::scheduler::Response`.
>
> It is going to be used to test operation state reconciliation.
>
>
> Diffs
> -
>
> include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661
> include/mesos/v1/scheduler/scheduler.proto
> 25ebcfcb07cb4fb928dcfdc242bb6509c9080491
> src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
> 60b17b9be74132c81532d22eba681feb899b22a3
> src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74
>
>
> Diff: https://reviews.apache.org/r/66460/diff/4/
>
>
> Testing
> ---
>
> `sudo bin/mesos-tests` on GNU/Linux
>
>
> Thanks,
>
> Gaston Kleiman
>
>
Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.
> On April 6, 2018, 2:13 p.m., Vinod Kone wrote: > > include/mesos/v1/scheduler.hpp > > Line 50 (original), 54-55 (patched) > > <https://reviews.apache.org/r/66460/diff/2/?file=1993421#file1993421line54> > > > > Since we don't guarantee backwards compat for this library, can we just > > update the signature of `send`? > > Gaston Kleiman wrote: > Would the libmesos binary then be compatible? Or would frameworks then > have to be recompiled using this new header in order to use new libmesos > binaries? > > Greg Mann wrote: > So, it sounds like if we broke the existing scheduler library interface, > that would mean that Java frameworks using the V0->V1 adapter would need to > simultaneously upgrade their Mesos JAR and libmesos. This is probably fine, > but we're checking with some framework developers to confirm. Updating the signature of `send` would break people who have installed schedulers from packages that don't bundle the Mesos JAR and libmesos together, e.g., people who installed Marathon/Chronos and Mesos from rpm or deb packages. - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/#review200670 ----------- On April 20, 2018, 5:03 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66460/ > --- > > (Updated April 20, 2018, 5:03 p.m.) > > > Review request for mesos, Greg Mann and Vinod Kone. > > > Bugs: MESOS-8192 > https://issues.apache.org/jira/browse/MESOS-8192 > > > Repository: mesos > > > Description > --- > > This patch adds a `call()` method to the scheduler library that allows > clients to send a `v1::scheduler::Call` to the master and receive a > `v1::scheduler::Response`. > > It is going to be used to test operation state reconciliation. > > > Diffs > - > > include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 > include/mesos/v1/scheduler/scheduler.proto > 25ebcfcb07cb4fb928dcfdc242bb6509c9080491 > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp > 60b17b9be74132c81532d22eba681feb899b22a3 > src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 > > > Diff: https://reviews.apache.org/r/66460/diff/4/ > > > Testing > --- > > `sudo bin/mesos-tests` on GNU/Linux > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66468: Added tests for operation status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66468/ --- (Updated April 20, 2018, 5:03 p.m.) Review request for mesos and Greg Mann. Changes --- Rebased + updates due to having changed the signature of the Scheduler library `call()` method. Bugs: MESOS-8191 https://issues.apache.org/jira/browse/MESOS-8191 Repository: mesos Description --- Added tests for operation status reconciliation. Diffs (updated) - src/Makefile.am 9d610bbe8108e5caa4a22977caa9dff07c8bb665 src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 src/tests/mesos.hpp 7356523a2a37858bbe866527425f35502ff5ad82 src/tests/operation_reconciliation_tests.cpp PRE-CREATION src/tests/storage_local_resource_provider_tests.cpp 2872f1aec1a7b94fc302a533f5ae9e1be9658087 Diff: https://reviews.apache.org/r/66468/diff/5/ Changes: https://reviews.apache.org/r/66468/diff/4-5/ Testing --- The new tests passed 1000 iterations on GNU/Linux. Thanks, Gaston Kleiman
Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/ --- (Updated April 20, 2018, 5:03 p.m.) Review request for mesos, Greg Mann and Vinod Kone. Changes --- Changed the signature of the new method. Bugs: MESOS-8192 https://issues.apache.org/jira/browse/MESOS-8192 Repository: mesos Description --- This patch adds a `call()` method to the scheduler library that allows clients to send a `v1::scheduler::Call` to the master and receive a `v1::scheduler::Response`. It is going to be used to test operation state reconciliation. Diffs (updated) - include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 include/mesos/v1/scheduler/scheduler.proto 25ebcfcb07cb4fb928dcfdc242bb6509c9080491 src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 60b17b9be74132c81532d22eba681feb899b22a3 src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 Diff: https://reviews.apache.org/r/66460/diff/4/ Changes: https://reviews.apache.org/r/66460/diff/3-4/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66696: Updated documentation for operation feedback.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66696/ --- (Updated April 20, 2018, 3:54 p.m.) Review request for mesos and Greg Mann. Changes --- Rebased. Repository: mesos Description --- Updated documentation for operation feedback. Diffs (updated) - docs/csi.md 2d96ee754b3fdcfb18a7516f30abf0086fa75137 docs/scheduler-http-api.md 3929c33781a152428338c4cdaf7dbc47da7c875e Diff: https://reviews.apache.org/r/66696/diff/4/ Changes: https://reviews.apache.org/r/66696/diff/3-4/ Testing --- Thanks, Gaston Kleiman
Re: Review Request 66466: Updated `RESERVE()` helper to allow specifying an operation ID.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66466/ --- (Updated April 20, 2018, 3:54 p.m.) Review request for mesos and Greg Mann. Changes --- Rebased. Bugs: MESOS-8191 https://issues.apache.org/jira/browse/MESOS-8191 Repository: mesos Description --- Updated `RESERVE()` helper to allow specifying an operation ID. Diffs (updated) - src/tests/mesos.hpp 7356523a2a37858bbe866527425f35502ff5ad82 Diff: https://reviews.apache.org/r/66466/diff/2/ Changes: https://reviews.apache.org/r/66466/diff/1-2/ Testing --- `sudo bin/mesos-tests` on GNU/Linux The tests added by https://reviews.apache.org/r/66468/ use this helper with an `OperationID`. Thanks, Gaston Kleiman
Re: Review Request 66467: Added a test helper for creating `RECONCILE_OPERATIONS` v1 calls.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66467/ --- (Updated April 20, 2018, 3:54 p.m.) Review request for mesos and Greg Mann. Changes --- Rebased. Bugs: MESOS-8191 https://issues.apache.org/jira/browse/MESOS-8191 Repository: mesos Description --- Added a test helper for creating `RECONCILE_OPERATIONS` v1 calls. Diffs (updated) - src/tests/mesos.hpp 7356523a2a37858bbe866527425f35502ff5ad82 Diff: https://reviews.apache.org/r/66467/diff/2/ Changes: https://reviews.apache.org/r/66467/diff/1-2/ Testing --- `sudo bin/mesos-tests` on GNU/Linux The tests added by https://reviews.apache.org/r/66468/ use this helper. Thanks, Gaston Kleiman
Re: Review Request 66465: Updated `using` statements in `tests/mesos.hpp`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66465/ --- (Updated April 20, 2018, 3:54 p.m.) Review request for mesos and Greg Mann. Changes --- Rebased. Bugs: MESOS-8191 https://issues.apache.org/jira/browse/MESOS-8191 Repository: mesos Description --- Updated `using` statements in `tests/mesos.hpp`. Diffs (updated) - src/tests/mesos.hpp 7356523a2a37858bbe866527425f35502ff5ad82 Diff: https://reviews.apache.org/r/66465/diff/2/ Changes: https://reviews.apache.org/r/66465/diff/1-2/ Testing --- `sudo bin/mesos-tests` on GNU/Linux The tests added by https://reviews.apache.org/r/66468/ use these statements. Thanks, Gaston Kleiman
Re: Review Request 66464: Implemented operation status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66464/ --- (Updated April 20, 2018, 3:53 p.m.) Review request for mesos and Greg Mann. Changes --- Addressed feedback + rebased. Bugs: MESOS-8191 https://issues.apache.org/jira/browse/MESOS-8191 Repository: mesos Description --- Implemented operation status reconciliation. Diffs (updated) - include/mesos/v1/scheduler/scheduler.proto 25ebcfcb07cb4fb928dcfdc242bb6509c9080491 src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a src/messages/messages.proto 556801d9ece3ada6d8e3cec51882ad50c27e24b2 Diff: https://reviews.apache.org/r/66464/diff/6/ Changes: https://reviews.apache.org/r/66464/diff/5-6/ Testing --- `sudo bin/mesos-tests` on GNU/Linux https://reviews.apache.org/r/66468/ adds new tests. Thanks, Gaston Kleiman
Re: Review Request 66462: Added new operation states to be used for status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66462/ --- (Updated April 20, 2018, 3:53 p.m.) Review request for mesos and Greg Mann. Changes --- Rebased. Bugs: MESOS-8191 https://issues.apache.org/jira/browse/MESOS-8191 Repository: mesos Description --- Added new operation states to be used for status reconciliation. Diffs (updated) - include/mesos/mesos.proto 9e24d3ea46edc21572e9226e2e76c7d55618db24 include/mesos/v1/mesos.proto 0f3fd8a2608b5edabc21f5fe5df9b70fc0fa8dc2 src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a src/slave/slave.cpp 9d2d1928b231044988f1855eb518448db38ff04f Diff: https://reviews.apache.org/r/66462/diff/5/ Changes: https://reviews.apache.org/r/66462/diff/4-5/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66463: Added a master metric for operations reconciliation messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66463/ --- (Updated April 20, 2018, 3:53 p.m.) Review request for mesos and Greg Mann. Changes --- Rebased. Bugs: MESOS-8191 https://issues.apache.org/jira/browse/MESOS-8191 Repository: mesos Description --- Added a master metric for operations reconciliation messages. Diffs (updated) - docs/monitoring.md 12e2103eb041e3e1b99bddafafcf4c615205fb0c docs/operator-http-api.md 10dcac83fa70da5760a5c231665d7498cc168622 src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb src/tests/master_tests.cpp be7a3cc6126d2fe7c56c49b3da5f6f4bf29657f5 Diff: https://reviews.apache.org/r/66463/diff/2/ Changes: https://reviews.apache.org/r/66463/diff/1-2/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66489: Cleaned up internal evolve functions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66489/ --- (Updated April 20, 2018, 3:47 p.m.) Review request for mesos and Greg Mann. Changes --- Rebased. Bugs: MESOS-8191 https://issues.apache.org/jira/browse/MESOS-8191 Repository: mesos Description --- This patch removes the `mesos::` prefix from declarations where possible, uses a consistent number of empty lines in the header, and reorders some declarations. Diffs (updated) - src/internal/evolve.hpp e00ac7175438f758b10c0aa9485fc29ffa4efa29 src/internal/evolve.cpp f598ea11b64bdbface5bf1f124a02ea876196b67 Diff: https://reviews.apache.org/r/66489/diff/2/ Changes: https://reviews.apache.org/r/66489/diff/1-2/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66459: Fixed bug in `Master::updateSlave()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66459/ --- (Updated April 20, 2018, 3:47 p.m.) Review request for mesos and Greg Mann. Changes --- Rebased. Repository: mesos Description --- A part of `Master::updateSlave()` doesn't account for operations created via the operator API; this patch fixes that. Diffs (updated) - src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a Diff: https://reviews.apache.org/r/66459/diff/2/ Changes: https://reviews.apache.org/r/66459/diff/1-2/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66461: Added an evolve function for `v1::scheduler::Response`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66461/ --- (Updated April 20, 2018, 3:47 p.m.) Review request for mesos and Greg Mann. Changes --- Rebased. Bugs: MESOS-8191 https://issues.apache.org/jira/browse/MESOS-8191 Repository: mesos Description --- Added an evolve function for `v1::scheduler::Response`. Diffs (updated) - src/internal/evolve.hpp e00ac7175438f758b10c0aa9485fc29ffa4efa29 src/internal/evolve.cpp f598ea11b64bdbface5bf1f124a02ea876196b67 Diff: https://reviews.apache.org/r/66461/diff/3/ Changes: https://reviews.apache.org/r/66461/diff/2-3/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66458: Fixed handling of operations in `master::recoverFramework()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66458/ --- (Updated April 20, 2018, 3:47 p.m.) Review request for mesos and Greg Mann. Changes --- Rebased. Repository: mesos Description --- `Master::recoverFramework()` only recovers operations affecting agent default resources. This patch makes it also recover operations affecting resources managed by resource providers. It also fixes a bug in which not just the corresponding operations, but all the ones affecting agent default resources will be added to the framework. Diffs (updated) - src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a Diff: https://reviews.apache.org/r/66458/diff/2/ Changes: https://reviews.apache.org/r/66458/diff/1-2/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66679: Made the master send operation status updates when dropping operations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66679/ --- (Updated April 20, 2018, 3:47 p.m.) Review request for mesos and Greg Mann. Changes --- Rebased. Bugs: MESOS-8781 https://issues.apache.org/jira/browse/MESOS-8781 Repository: mesos Description --- This patch makes the master send an operation status update to the framework with status `OPERATION_ERROR` when an operation with an operation ID is dropped. Diffs (updated) - src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a Diff: https://reviews.apache.org/r/66679/diff/3/ Changes: https://reviews.apache.org/r/66679/diff/2-3/ Testing --- `sudo bin/mesos-tests.sh` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66696: Updated documentation for operation feedback.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66696/ --- (Updated April 19, 2018, 11:12 a.m.) Review request for mesos and Greg Mann. Changes --- Addressed feedback, updated the CSI doc. Summary (updated) - Updated documentation for operation feedback. Repository: mesos Description (updated) --- Updated documentation for operation feedback. Diffs (updated) - docs/csi.md 2d96ee754b3fdcfb18a7516f30abf0086fa75137 docs/scheduler-http-api.md 3929c33781a152428338c4cdaf7dbc47da7c875e Diff: https://reviews.apache.org/r/66696/diff/3/ Changes: https://reviews.apache.org/r/66696/diff/2-3/ Testing --- Thanks, Gaston Kleiman
Re: Review Request 66696: Updated Scheduler HTTP API doc for operation feedback.
> On April 18, 2018, 2:51 p.m., Greg Mann wrote: > > docs/scheduler-http-api.md > > Lines 426-428 (patched) > > <https://reviews.apache.org/r/66696/diff/1/?file=2005687#file2005687line426> > > > > This call requires the `Accept` header, right? It technically doesn't require the header, but if one is set, it should contain json or protobuf. Anyway, I added the header here for consistency with the other examples. > On April 18, 2018, 2:51 p.m., Greg Mann wrote: > > docs/scheduler-http-api.md > > Lines 434-439 (patched) > > <https://reviews.apache.org/r/66696/diff/1/?file=2005687#file2005687line434> > > > > Should we include the `resource_provider_id` field here, since the MVP > > is RP resource-only? Nope, the resource provider id field is not needed (and will be ignored) when reconciling operations. It is there for when we support ERPS. - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66696/#review201466 ------- On April 18, 2018, 3:15 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66696/ > --- > > (Updated April 18, 2018, 3:15 p.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > --- > > Updated Scheduler HTTP API doc for operation feedback. > > > Diffs > - > > docs/scheduler-http-api.md 3929c33781a152428338c4cdaf7dbc47da7c875e > > > Diff: https://reviews.apache.org/r/66696/diff/2/ > > > Testing > --- > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66696: Updated Scheduler HTTP API doc for operation feedback.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66696/ --- (Updated April 18, 2018, 3:15 p.m.) Review request for mesos and Greg Mann. Changes --- Addressed feedeback. Repository: mesos Description --- Updated Scheduler HTTP API doc for operation feedback. Diffs (updated) - docs/scheduler-http-api.md 3929c33781a152428338c4cdaf7dbc47da7c875e Diff: https://reviews.apache.org/r/66696/diff/2/ Changes: https://reviews.apache.org/r/66696/diff/1-2/ Testing --- Thanks, Gaston Kleiman
Re: Review Request 66464: Implemented operation status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66464/ --- (Updated April 18, 2018, 2:36 p.m.) Review request for mesos and Greg Mann. Changes --- Made checking of the HTTP accept header conditional. Repository: mesos Description --- Implemented operation status reconciliation. Diffs (updated) - src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a Diff: https://reviews.apache.org/r/66464/diff/5/ Changes: https://reviews.apache.org/r/66464/diff/4-5/ Testing --- `sudo bin/mesos-tests` on GNU/Linux https://reviews.apache.org/r/66468/ adds new tests. Thanks, Gaston Kleiman
Re: Review Request 66679: Made the master send operation status updates when dropping operations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66679/ --- (Updated April 18, 2018, 1:38 p.m.) Review request for mesos and Greg Mann. Changes --- Addressed feedback. Bugs: MESOS-8781 https://issues.apache.org/jira/browse/MESOS-8781 Repository: mesos Description --- This patch makes the master send an operation status update to the framework with status `OPERATION_ERROR` when an operation with an operation ID is dropped. Diffs (updated) - src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a Diff: https://reviews.apache.org/r/66679/diff/2/ Changes: https://reviews.apache.org/r/66679/diff/1-2/ Testing --- `sudo bin/mesos-tests.sh` on GNU/Linux Thanks, Gaston Kleiman
Review Request 66696: Updated Scheduler HTTP API doc for operation feedback.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66696/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- Updated Scheduler HTTP API doc for operation feedback. Diffs - docs/scheduler-http-api.md 3929c33781a152428338c4cdaf7dbc47da7c875e Diff: https://reviews.apache.org/r/66696/diff/1/ Testing --- Thanks, Gaston Kleiman
Review Request 66679: Made the master send operation status updates when dropping operations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66679/ --- Review request for mesos and Greg Mann. Bugs: MESOS-8781 https://issues.apache.org/jira/browse/MESOS-8781 Repository: mesos Description --- This patch makes the master send an operation status update to the framework with status `OPERATION_ERROR` when an operation with an operation ID is dropped. Diffs - src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a Diff: https://reviews.apache.org/r/66679/diff/1/ Testing --- `sudo bin/mesos-tests.sh` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66220: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66220/#review201355 --- src/tests/persistent_volume_tests.cpp Lines 474-475 (patched) <https://reviews.apache.org/r/66220/#comment282536> I see that Benjamin recently added this line and the corresponding `AWAIT_READY` to many tests. However I don't think it is necessary here; I'll check with Benjamin to see if we can also remove it from other tests in which it is not necessary. src/tests/persistent_volume_tests.cpp Lines 502 (patched) <https://reviews.apache.org/r/66220/#comment282539> Nit: `Offer offer = offersBeforeCreate->at(0);` If this test is split into smaller ones, then you can also probable make `offer` a const ref. - Gaston Kleiman On April 11, 2018, 2:19 p.m., Zhitao Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66220/ > --- > > (Updated April 11, 2018, 2:19 p.m.) > > > Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann. > > > Bugs: MESOS-4965 > https://issues.apache.org/jira/browse/MESOS-4965 > > > Repository: mesos > > > Description > --- > > Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations. > > > Diffs > - > > src/tests/persistent_volume_tests.cpp > 4edf781711d9efdb994114aeb6289b6af750b87a > > > Diff: https://reviews.apache.org/r/66220/diff/4/ > > > Testing > --- > > > Thanks, > > Zhitao Li > >
Re: Review Request 66615: Adopted `createTask` helper API in Docker containerizer tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66615/#review201257 --- Ship it! Ship It! - Gaston Kleiman On April 16, 2018, 9:51 a.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66615/ > --- > > (Updated April 16, 2018, 9:51 a.m.) > > > Review request for mesos, Andrei Budnik, Gaston Kleiman, Greg Mann, and > Zhitao Li. > > > Repository: mesos > > > Description > --- > > The Docker containerizer tests were manually creating the `TaskInfo` > for the tasks they wanted to launch. We can remove some of that > boilerplate by adopting the `createTask` helper. > > > Diffs > - > > src/tests/containerizer/docker_containerizer_tests.cpp > 847258daadf3c37d9071151616b18fc79d850ce8 > src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e > > > Diff: https://reviews.apache.org/r/66615/diff/2/ > > > Testing > --- > > sudo make check (Fedora 27). > > > Thanks, > > James Peach > >
Re: Review Request 66050: Implemented grow and shrink of persistent volumes.
> On April 16, 2018, 10:38 a.m., Gaston Kleiman wrote:
> > src/master/validation.cpp
> > Lines 2348-2351 (patched)
> > <https://reviews.apache.org/r/66050/diff/10/?file=2002407#file2002407line2348>
> >
> > Isn't it easier to do the following?
> >
> > ```
> > if (growVoluem.addition.scalar.value() <= 0) {
> > ...
> > ```
>
> Chun-Hung Hsiao wrote:
> The `Scalar` comparison contains a fixed-point conversion:
> https://github.com/apache/mesos/blob/master/src/common/values.cpp#L103
This isn't obvious to the uninformed reader (aka me). Can we add a comment here?
- Gaston
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66050/#review201229
---
On April 16, 2018, 10:09 a.m., Zhitao Li wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
>
> (Updated April 16, 2018, 10:09 a.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
>
>
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
>
>
> Repository: mesos
>
>
> Description
> ---
>
> The new offer operations are implemented as speculative operations, but
> we will use validation to make them non-speculative on API level so that
> we can transition later without a breaking change.
>
>
> Diffs
> -
>
> src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080
> src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518
> src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a
> src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf
> src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d
>
>
> Diff: https://reviews.apache.org/r/66050/diff/10/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66344: Supported non-speculative operations on agent default resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66344/#review201237 --- src/slave/slave.cpp Lines 4328 (patched) <https://reviews.apache.org/r/66344/#comment282375> The agent should retry this update for as long as possible, so it should set a status UUID. - Gaston Kleiman On March 28, 2018, 2:41 p.m., Zhitao Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66344/ > --- > > (Updated March 28, 2018, 2:41 p.m.) > > > Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann. > > > Bugs: MESOS-4965 > https://issues.apache.org/jira/browse/MESOS-4965 > > > Repository: mesos > > > Description > --- > > We use `getResourceConversions` to calculate `conversions`, > apply that to agent default resources if not on resource provider, > and send `UpdateOperationStatusMessage` message back to master. > > > Diffs > - > > src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb > > > Diff: https://reviews.apache.org/r/66344/diff/3/ > > > Testing > --- > > > Thanks, > > Zhitao Li > >
Re: Review Request 66637: Added a `createDockerInfo` helper API in Docker containerizer tests.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66637/#review201234
---
Ship it!
Thanks again for the cleanup!
Just a random comment, we could start doing:
```
*task.mutable_container() = createDockerInfo("alpine");
```
Instead of:
```
task.mutable_container()->CopyFrom(createDockerInfo("alpine"));
```
But that would not be consistent with what we do in most places in our tests,
so I didn't open an issue =).
- Gaston Kleiman
On April 16, 2018, 10:31 a.m., James Peach wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66637/
> ---
>
> (Updated April 16, 2018, 10:31 a.m.)
>
>
> Review request for mesos, Andrei Budnik, Gaston Kleiman, Greg Mann, and
> Zhitao Li.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> The Docker containerizer tests all create a Docker `ContainerInfo`.
> We can reduce the amount of copy/paste boilerplate by adding a
> simple helper function to create a `ContainerInfo` with a Docker
> image.
>
>
> Diffs
> -
>
> src/tests/containerizer/docker_containerizer_tests.cpp
> 847258daadf3c37d9071151616b18fc79d850ce8
>
>
> Diff: https://reviews.apache.org/r/66637/diff/2/
>
>
> Testing
> ---
>
> sudo make check (Fedora 27)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 66050: Implemented grow and shrink of persistent volumes.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66050/#review201229
---
src/master/master.cpp
Lines 4931-4932 (patched)
<https://reviews.apache.org/r/66050/#comment282362>
I'd change this to:
```
"Agent " + stringify(slave) + " does not have the required
RESOURCE_PROVIDER capability"
```
To be consistent with what `Master::accept()` returns when an operation ID
is specified, but the agent doesn't support `RESOURCE_PROVIDER`.
src/master/master.cpp
Lines 4973 (patched)
<https://reviews.apache.org/r/66050/#comment282363>
Shouldn't it be `on agent`?
src/master/master.cpp
Lines 5472 (patched)
<https://reviews.apache.org/r/66050/#comment282364>
I don't think we need to add this line in this patch.
I did notice that we are not consistent in always adding an empty line
after the `drop(...)`. We might want to do a sweep on this function.
src/master/validation.cpp
Lines 2337 (patched)
<https://reviews.apache.org/r/66050/#comment282367>
We don't normally include the type of operation in the error.
If we do decide to include it for these new operations, I'd write it in
upper case.
src/master/validation.cpp
Lines 2348-2351 (patched)
<https://reviews.apache.org/r/66050/#comment282365>
Isn't it easier to do the following?
```
if (growVoluem.addition.scalar.value() <= 0) {
...
```
- Gaston Kleiman
On April 16, 2018, 10:09 a.m., Zhitao Li wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
>
> (Updated April 16, 2018, 10:09 a.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
>
>
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
>
>
> Repository: mesos
>
>
> Description
> ---
>
> The new offer operations are implemented as speculative operations, but
> we will use validation to make them non-speculative on API level so that
> we can transition later without a breaking change.
>
>
> Diffs
> -
>
> src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080
> src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518
> src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a
> src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf
> src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d
>
>
> Diff: https://reviews.apache.org/r/66050/diff/10/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66637: Added a `createDockerInfo` helper API in Docker containerizer tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66637/#review201225 --- src/tests/containerizer/docker_containerizer_tests.cpp Lines 107-111 (patched) <https://reviews.apache.org/r/66637/#comment282357> Nit: I'd do: ``` ContainerInfo::DockerInfo* dockerInfo = containerInfo.mutable_docker(); dockerInfo->set_image(imageName); return containerInfo; ``` src/tests/containerizer/docker_containerizer_tests.cpp Line 378 (original) <https://reviews.apache.org/r/66637/#comment282358> The default networking is `HOST`, so this line shouldn't be removed. - Gaston Kleiman On April 16, 2018, 9:52 a.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66637/ > --- > > (Updated April 16, 2018, 9:52 a.m.) > > > Review request for mesos, Andrei Budnik, Gaston Kleiman, Greg Mann, and > Zhitao Li. > > > Repository: mesos > > > Description > --- > > The Docker containerizer tests all create a Docker `ContainerInfo`. > We can reduce the amount of copy/paste boilerplate by adding a > simple helper function to create a `ContainerInfo` with a Docker > image. > > > Diffs > - > > src/tests/containerizer/docker_containerizer_tests.cpp > 847258daadf3c37d9071151616b18fc79d850ce8 > > > Diff: https://reviews.apache.org/r/66637/diff/1/ > > > Testing > --- > > sudo make check (Fedora 27) > > > Thanks, > > James Peach > >
Re: Review Request 66050: Implemented grow and shrink of persistent volumes.
> On March 27, 2018, 12:14 p.m., Greg Mann wrote:
> > src/common/resources_utils.cpp
> > Lines 696-698 (patched)
> > <https://reviews.apache.org/r/66050/diff/6/?file=1988969#file1988969line696>
> >
> > No period at the end of error messages. Here and below.
Are you sure? That is not consistent with the error messages that this method
will return for the other operations.
For example:
```
if (!operation->has_launch()) {
return Error(
"A LAUNCH offer operation must have"
" the Offer.Operation.launch field set.");
}
```
- Gaston
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66050/#review200055
---
On April 9, 2018, 9:18 p.m., Zhitao Li wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
>
> (Updated April 9, 2018, 9:18 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
>
>
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
>
>
> Repository: mesos
>
>
> Description
> ---
>
> The new offer operations are implemented as speculative operations, but
> we will use validation to make them non-speculative on API level so that
> we can transition later without a breaking change.
>
>
> Diffs
> -
>
> src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080
> src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518
> src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8
> src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf
> src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d
> src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e
> src/tests/persistent_volume_tests.cpp
> 4edf781711d9efdb994114aeb6289b6af750b87a
> src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d
>
>
> Diff: https://reviews.apache.org/r/66050/diff/8/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66464: Implemented operation status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66464/ --- (Updated April 13, 2018, 5:51 p.m.) Review request for mesos and Greg Mann. Changes --- Addressed Greg's feedback. Repository: mesos Description --- Implemented operation status reconciliation. Diffs (updated) - src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 Diff: https://reviews.apache.org/r/66464/diff/4/ Changes: https://reviews.apache.org/r/66464/diff/3-4/ Testing --- `sudo bin/mesos-tests` on GNU/Linux https://reviews.apache.org/r/66468/ adds new tests. Thanks, Gaston Kleiman
Re: Review Request 66615: Adopted `createTask` helper API in Docker containerizer tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66615/#review201146 --- Thanks for the cleanup! I think we could get rid of quite some boilerplate with a helper that creates a `ContainerInfo::DockerInfo`. src/tests/containerizer/docker_containerizer_tests.cpp Line 265 (original), 260 (patched) <https://reviews.apache.org/r/66615/#comment282166> Nit: I'd add an empty line above this one. Here and in the other tests. - Gaston Kleiman On April 13, 2018, 4:57 p.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66615/ > --- > > (Updated April 13, 2018, 4:57 p.m.) > > > Review request for mesos, Andrei Budnik, Gaston Kleiman, Greg Mann, and > Zhitao Li. > > > Repository: mesos > > > Description > --- > > The Docker containerizer tests were manually creating the `TaskInfo` > for the tasks they wanted to launch. We can remove some of that > boilerplate by adopting the `createTask` helper. > > > Diffs > - > > src/tests/containerizer/docker_containerizer_tests.cpp > 847258daadf3c37d9071151616b18fc79d850ce8 > src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e > > > Diff: https://reviews.apache.org/r/66615/diff/1/ > > > Testing > --- > > sudo make check (Fedora 27). > > > Thanks, > > James Peach > >
Re: Review Request 66462: Added new operation states to be used for status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66462/ --- (Updated April 12, 2018, 9:59 a.m.) Review request for mesos and Greg Mann. Changes --- Addressed feedback from Chun. Repository: mesos Description --- Added new operation states to be used for status reconciliation. Diffs (updated) - include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 Diff: https://reviews.apache.org/r/66462/diff/4/ Changes: https://reviews.apache.org/r/66462/diff/3-4/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66462: Added new operation states to be used for status reconciliation.
> On April 11, 2018, 7:06 p.m., Chun-Hung Hsiao wrote:
> > Are all of these necessary for now?
Yeah, they are all generated/sent by the {{ReconcileOperations}} handler.
> On April 11, 2018, 7:06 p.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 8034-8038 (patched)
> > <https://reviews.apache.org/r/66462/diff/3/?file=1996019#file1996019line8035>
> >
> > Move these outside the block.
lol, good catch, thanks!!!
- Gaston
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66462/#review200957
-------
On April 11, 2018, 10:58 a.m., Gaston Kleiman wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66462/
> ---
>
> (Updated April 11, 2018, 10:58 a.m.)
>
>
> Review request for mesos and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Added new operation states to be used for status reconciliation.
>
>
> Diffs
> -
>
> include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907
> include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95
> src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080
> src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8
> src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5
>
>
> Diff: https://reviews.apache.org/r/66462/diff/3/
>
>
> Testing
> ---
>
> `sudo bin/mesos-tests` on GNU/Linux
>
>
> Thanks,
>
> Gaston Kleiman
>
>
Re: Review Request 66462: Added new operation states to be used for status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66462/ --- (Updated April 11, 2018, 10:58 a.m.) Review request for mesos and Greg Mann. Changes --- Improved comments. Repository: mesos Description --- Added new operation states to be used for status reconciliation. Diffs (updated) - include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 Diff: https://reviews.apache.org/r/66462/diff/3/ Changes: https://reviews.apache.org/r/66462/diff/2-3/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.
> On April 10, 2018, 1:45 p.m., Benjamin Mahler wrote: > > include/mesos/v1/scheduler.hpp > > Lines 102-114 (patched) > > <https://reviews.apache.org/r/66460/diff/3/?file=1993428#file1993428line102> > > > > Hm.. why does this return an `Option`? > > > > If this is mesos Response rather than http::Response, aren't we losing > > information about which http code came back? (e.g. 400, 401, etc). > > Gaston Kleiman wrote: > If the request is not successful and the http code is neither 200 nor > 202, the method will return an error that includes the http code and the body > of the HTTP response. > > It returns a `scheduler::Response` in order to abstract the consumer from > the transport layer and prevent it from having to understand and deserialize > HTTP responses. > > Benjamin Mahler wrote: > How do I program against that? Parse the failure.message() into an > http::Response? > > It looks like we need: `Future<variant<http::Response, > master::Response>>` where master::Response is returned for 200 OK? (If we > want to handle parsing into master::Response for them. > > Gaston Kleiman wrote: > Most scheduler API calls don't return a `scheduler::Response`, so we'd > need to use `Future<variant<http::Response, Option>>`. > > However that would only give the consumer access to the `http::Response` > if the request fails. We might want to return an enum that always contains > the `http::Response` and that also includes an `Option`. > > Note that the current call `send()` is a `void` method, so we've been > able to avoid using the `http::Response` sent by the master. > > Greg Mann wrote: > This library does not expose the transport layer to the client at all, > and the current patch maintains that convention. > > If we're going to expose the HTTP response, I think Gastón's suggestion > of including it unconditionally makes sense. > > Without the HTTP response, yes I think the client would have to parse the > failure message to deduce the error. Perhaps this is a suitable time to start > exposing HTTP here? I would be fine with that. > > Benjamin Mahler wrote: > > `Future<variant<http::Response, Option>>` > > Are you sure you need the option? I think the client can always decode a > scheduler::Response, because when the master sends nothing back, the client > can successfully decode an empty `scheduler::Response`, it just won't have > `Response::type` set? In any case, the inconsistency here seems a bit > confusing? > > > This library does not expose the transport layer to the client at all, > and the current patch maintains that convention. > > HTTP is in the application layer rather than tranport layer (to be > pendantic :)), and that makes sense here since it encodes some of the > application level responses from the master rather than all application > behavior being inside `master::Response`. E.g. 400 Bad Request > > > Perhaps this is a suitable time to start exposing HTTP here? I would be > fine with that. > > It's either that, or we "abstract" response codes / bodies out into some > other structure, but it doesn't seem maintainable as more http response codes > get returned by the master? > > Anyway, sorry to throw a wrench in here :) I just want to avoid parsing > strings to program against the master. (I'm assuming there will be some > response codes that the scheduler will react differently to: e.g. 400 > shouldn't be retried, whereas 503 should?) > > `Future<variant<http::Response, Option>>` > Are you sure you need the option? I think the client can always decode a > scheduler::Response, because when the master sends nothing back, the client > can successfully decode an empty scheduler::Response, it just won't have > Response::type set? In any case, the inconsistency here seems a bit confusing? I think that using an option makes it semantically clearer that the master might not respond to a scheduler API call with a `scheduler::Response`. I also find checking whether an option is set or not less awkward and more readable than checking whether `scheduler::Response::type` is set. An HTTP will always be returned if the future didn't fail, but a `scheduler::Response` is optional and the master in most cases doesn't respond with one. I don't think that we need symmetry/consistency in the method signature. > > Perhaps this is a suitable time to start exposing HTTP here? I would be > > fine with that. > It's either that, or we "abstract" response codes /
Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/ --- (Updated April 10, 2018, 7:25 p.m.) Review request for mesos, Greg Mann and Vinod Kone. Changes --- Updated metadata. Bugs: MESOS-8192 https://issues.apache.org/jira/browse/MESOS-8192 Repository: mesos Description --- This patch adds a `call()` method to the scheduler library that allows clients to send a `v1::scheduler::Call` to the master and receive a `v1::scheduler::Response`. It is going to be used to test operation state reconciliation. Diffs - include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 60b17b9be74132c81532d22eba681feb899b22a3 src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 Diff: https://reviews.apache.org/r/66460/diff/3/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66468: Added tests for operation status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66468/ --- (Updated April 10, 2018, 2:56 p.m.) Review request for mesos and Greg Mann. Changes --- Disabled on Windows a test that requires the replicated log, which isn't supported on Windwos. Repository: mesos Description --- Added tests for operation status reconciliation. Diffs (updated) - src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 src/tests/operation_reconciliation_tests.cpp PRE-CREATION src/tests/storage_local_resource_provider_tests.cpp 2872f1aec1a7b94fc302a533f5ae9e1be9658087 Diff: https://reviews.apache.org/r/66468/diff/4/ Changes: https://reviews.apache.org/r/66468/diff/3-4/ Testing --- The new tests passed 1000 iterations on GNU/Linux. Thanks, Gaston Kleiman
Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.
> On April 10, 2018, 1:45 p.m., Benjamin Mahler wrote: > > include/mesos/v1/scheduler.hpp > > Lines 102-114 (patched) > > <https://reviews.apache.org/r/66460/diff/3/?file=1993428#file1993428line102> > > > > Hm.. why does this return an `Option`? > > > > If this is mesos Response rather than http::Response, aren't we losing > > information about which http code came back? (e.g. 400, 401, etc). > > Gaston Kleiman wrote: > If the request is not successful and the http code is neither 200 nor > 202, the method will return an error that includes the http code and the body > of the HTTP response. > > It returns a `scheduler::Response` in order to abstract the consumer from > the transport layer and prevent it from having to understand and deserialize > HTTP responses. > > Benjamin Mahler wrote: > How do I program against that? Parse the failure.message() into an > http::Response? > > It looks like we need: `Future<variant<http::Response, > master::Response>>` where master::Response is returned for 200 OK? (If we > want to handle parsing into master::Response for them. Most scheduler API calls don't return a `scheduler::Response`, so we'd need to use `Future<variant<http::Response, Option>>`. However that would only give the consumer access to the `http::Response` if the request fails. We might want to return an enum that always contains the `http::Response` and that also includes an `Option`. Note that the current call `send()` is a `void` method, so we've been able to avoid using the `http::Response` sent by the master. - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/#review200842 --- On April 6, 2018, 2:17 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66460/ > --- > > (Updated April 6, 2018, 2:17 p.m.) > > > Review request for mesos, Greg Mann and Vinod Kone. > > > Repository: mesos > > > Description > --- > > This patch adds a `call()` method to the scheduler library that allows > clients to send a `v1::scheduler::Call` to the master and receive a > `v1::scheduler::Response`. > > It is going to be used to test operation state reconciliation. > > > Diffs > - > > include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp > 60b17b9be74132c81532d22eba681feb899b22a3 > src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 > > > Diff: https://reviews.apache.org/r/66460/diff/3/ > > > Testing > --- > > `sudo bin/mesos-tests` on GNU/Linux > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.
> On April 10, 2018, 1:45 p.m., Benjamin Mahler wrote: > > include/mesos/v1/scheduler.hpp > > Lines 102-114 (patched) > > <https://reviews.apache.org/r/66460/diff/3/?file=1993428#file1993428line102> > > > > Hm.. why does this return an `Option`? > > > > If this is mesos Response rather than http::Response, aren't we losing > > information about which http code came back? (e.g. 400, 401, etc). If the request is not successful and the http code is neither 200 nor 202, the method will return an error that includes the http code and the body of the HTTP response. It returns a `scheduler::Response` in order to abstract the consumer from the transport layer and prevent it from having to understand and deserialize HTTP responses. - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/#review200842 ----------- On April 6, 2018, 2:17 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66460/ > --- > > (Updated April 6, 2018, 2:17 p.m.) > > > Review request for mesos, Greg Mann and Vinod Kone. > > > Repository: mesos > > > Description > --- > > This patch adds a `call()` method to the scheduler library that allows > clients to send a `v1::scheduler::Call` to the master and receive a > `v1::scheduler::Response`. > > It is going to be used to test operation state reconciliation. > > > Diffs > - > > include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp > 60b17b9be74132c81532d22eba681feb899b22a3 > src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 > > > Diff: https://reviews.apache.org/r/66460/diff/3/ > > > Testing > --- > > `sudo bin/mesos-tests` on GNU/Linux > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66468: Added tests for operation status reconciliation.
> On April 9, 2018, 5:08 p.m., Greg Mann wrote: > > src/tests/operation_reconciliation_tests.cpp > > Lines 364-365 (patched) > > <https://reviews.apache.org/r/66468/diff/1/?file=1994432#file1994432line364> > > > > Is there a reason not to do this with a `for` loop? > > Gaston Kleiman wrote: > I copied & pasted this from > https://github.com/apache/mesos/blob/be47e96e727f07758ff9b8ba1c23bbec2a489cd6/src/tests/reconciliation_tests.cpp#L1221-L1230, > so consistency with that file would be the only reason =). > > Doing this with a `for` loop calls `FUTURE_MESSAGE` and `Clock::advance` > one extra time, but that doesn't really hurt, so I converted the `while` loop > into a `for` loop. Let me know if you like it this way or if you'd prefer me > to revert this change. I changed it back to a `while` loop as discussed in chat. - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66468/#review200769 ------- On April 10, 2018, 11:45 a.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66468/ > --- > > (Updated April 10, 2018, 11:45 a.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > --- > > Added tests for operation status reconciliation. > > > Diffs > - > > src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a > src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 > src/tests/operation_reconciliation_tests.cpp PRE-CREATION > src/tests/storage_local_resource_provider_tests.cpp > 2872f1aec1a7b94fc302a533f5ae9e1be9658087 > > > Diff: https://reviews.apache.org/r/66468/diff/3/ > > > Testing > --- > > The new tests passed 1000 iterations on GNU/Linux. > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66468: Added tests for operation status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66468/ --- (Updated April 10, 2018, 11:45 a.m.) Review request for mesos and Greg Mann. Repository: mesos Description --- Added tests for operation status reconciliation. Diffs (updated) - src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 src/tests/operation_reconciliation_tests.cpp PRE-CREATION src/tests/storage_local_resource_provider_tests.cpp 2872f1aec1a7b94fc302a533f5ae9e1be9658087 Diff: https://reviews.apache.org/r/66468/diff/3/ Changes: https://reviews.apache.org/r/66468/diff/2-3/ Testing --- The new tests passed 1000 iterations on GNU/Linux. Thanks, Gaston Kleiman
Re: Review Request 66489: Cleaned up internal evolve functions.
> On April 10, 2018, 11:39 a.m., Greg Mann wrote: > > src/internal/evolve.hpp > > Line 152 (original), 152-153 (patched) > > <https://reviews.apache.org/r/66489/diff/1/?file=1993600#file1993600line158> > > > > Can we get rid of the `mesos::` here as well? Nope, if we get rid of `mesos::` here, it will complain about not finding `mesos::internal::master::Response`: ``` ../../src/internal/evolve.hpp:152:40: error: ‘Event’ in namespace ‘mesos::internal::master’ does not name a type v1::master::Event evolve(const master::Event& event); ^ ../../src/internal/evolve.hpp:153:43: error: ‘Response’ in namespace ‘mesos::internal::master’ does not name a type v1::master::Response evolve(const master::Response& response); ^~~~ ``` - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66489/#review200833 ------- On April 6, 2018, 5:16 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66489/ > --- > > (Updated April 6, 2018, 5:16 p.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > --- > > This patch removes the `mesos::` prefix from declarations where > possible, uses a consistent number of empty lines in the header, > and reorders some declarations. > > > Diffs > - > > src/internal/evolve.hpp e00ac7175438f758b10c0aa9485fc29ffa4efa29 > src/internal/evolve.cpp f598ea11b64bdbface5bf1f124a02ea876196b67 > > > Diff: https://reviews.apache.org/r/66489/diff/1/ > > > Testing > --- > > `sudo bin/mesos-tests` on GNU/Linux > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66468: Added tests for operation status reconciliation.
> On April 9, 2018, 5:08 p.m., Greg Mann wrote: > > src/tests/operation_reconciliation_tests.cpp > > Lines 364-365 (patched) > > <https://reviews.apache.org/r/66468/diff/1/?file=1994432#file1994432line364> > > > > Is there a reason not to do this with a `for` loop? I copied & pasted this from https://github.com/apache/mesos/blob/be47e96e727f07758ff9b8ba1c23bbec2a489cd6/src/tests/reconciliation_tests.cpp#L1221-L1230, so consistency with that file would be the only reason =). Doing this with a `for` loop calls `FUTURE_MESSAGE` and `Clock::advance` one extra time, but that doesn't really hurt, so I converted the `while` loop into a `for` loop. Let me know if you like it this way or if you'd prefer me to revert this change. - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66468/#review200769 --- On April 9, 2018, 5:30 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66468/ > --- > > (Updated April 9, 2018, 5:30 p.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > --- > > Added tests for operation status reconciliation. > > > Diffs > - > > src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a > src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 > src/tests/operation_reconciliation_tests.cpp PRE-CREATION > src/tests/storage_local_resource_provider_tests.cpp > 2872f1aec1a7b94fc302a533f5ae9e1be9658087 > > > Diff: https://reviews.apache.org/r/66468/diff/2/ > > > Testing > --- > > The new tests passed 1000 iterations on GNU/Linux. > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66468: Added tests for operation status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66468/ --- (Updated April 9, 2018, 5:30 p.m.) Review request for mesos and Greg Mann. Changes --- Addressed Greg's comments. Repository: mesos Description --- Added tests for operation status reconciliation. Diffs (updated) - src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 src/tests/operation_reconciliation_tests.cpp PRE-CREATION src/tests/storage_local_resource_provider_tests.cpp 2872f1aec1a7b94fc302a533f5ae9e1be9658087 Diff: https://reviews.apache.org/r/66468/diff/2/ Changes: https://reviews.apache.org/r/66468/diff/1-2/ Testing --- The new tests passed 1000 iterations on GNU/Linux. Thanks, Gaston Kleiman
Review Request 66468: Added tests for operation status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66468/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- Added tests for operation status reconciliation. Diffs - src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 src/tests/operation_reconciliation_tests.cpp PRE-CREATION src/tests/storage_local_resource_provider_tests.cpp 2872f1aec1a7b94fc302a533f5ae9e1be9658087 Diff: https://reviews.apache.org/r/66468/diff/1/ Testing --- The new tests passed 1000 iterations on GNU/Linux. Thanks, Gaston Kleiman
Re: Review Request 66464: Implemented operation status reconciliation.
> On April 9, 2018, 12:04 p.m., Greg Mann wrote: > > src/master/http.cpp > > Lines 5097 (patched) > > <https://reviews.apache.org/r/66464/diff/1/?file=1992581#file1992581line5102> > > > > s/reconcileOperations/std::move(reconcileOperations)/ > > Gaston Kleiman wrote: > Do you think that we need to use std::move to cast it as an rvalue? > > > It is already an rvalue per the method signature: > > ``` > Future Master::Http::reconcileOperations( > scheduler::Response::ReconcileOperations&& reconcileOperations, > ContentType contentType) const > ``` After writing a small program to test this, I can confirm that I was wrong. Good catch! Thanks =). - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66464/#review200680 ------- On April 9, 2018, 2:30 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66464/ > --- > > (Updated April 9, 2018, 2:30 p.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > --- > > Implemented operation status reconciliation. > > > Diffs > - > > src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 > src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 > src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 > > > Diff: https://reviews.apache.org/r/66464/diff/3/ > > > Testing > --- > > `sudo bin/mesos-tests` on GNU/Linux > > https://reviews.apache.org/r/66468/ adds new tests. > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66464: Implemented operation status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66464/ --- (Updated April 9, 2018, 2:30 p.m.) Review request for mesos and Greg Mann. Changes --- Addressed Greg's feedback. Repository: mesos Description --- Implemented operation status reconciliation. Diffs (updated) - src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 Diff: https://reviews.apache.org/r/66464/diff/2/ Changes: https://reviews.apache.org/r/66464/diff/1-2/ Testing --- `sudo bin/mesos-tests` on GNU/Linux https://reviews.apache.org/r/66468/ adds new tests. Thanks, Gaston Kleiman
Re: Review Request 66464: Implemented operation status reconciliation.
> On April 9, 2018, 12:04 p.m., Greg Mann wrote: > > src/master/http.cpp > > Lines 5097 (patched) > > <https://reviews.apache.org/r/66464/diff/1/?file=1992581#file1992581line5102> > > > > s/reconcileOperations/std::move(reconcileOperations)/ Do you think that we need to use std::move to cast it as an rvalue? It is already an rvalue per the method signature: ``` Future Master::Http::reconcileOperations( scheduler::Response::ReconcileOperations&& reconcileOperations, ContentType contentType) const ``` > On April 9, 2018, 12:04 p.m., Greg Mann wrote: > > src/master/master.hpp > > Lines 2556 (patched) > > <https://reviews.apache.org/r/66464/diff/1/?file=1992582#file1992582line2556> > > > > A return type of `Try<Operation*>` seems semanticallly more appropriate > > here? > > > > Also, let's pass the id as a const ref. I agree that id should be const OperationID&. Regarding returning a Try, what error would it return? I see this as semantically equivalent to a get method on a hashmap, so an Option seems to be a better fit. - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66464/#review200680 --- On April 4, 2018, 5:47 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66464/ > --- > > (Updated April 4, 2018, 5:47 p.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > --- > > Implemented operation status reconciliation. > > > Diffs > - > > src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 > src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 > src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c > > > Diff: https://reviews.apache.org/r/66464/diff/1/ > > > Testing > --- > > `sudo bin/mesos-tests` on GNU/Linux > > https://reviews.apache.org/r/66468/ adds new tests. > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66464: Implemented operation status reconciliation.
> On April 9, 2018, 12:04 p.m., Greg Mann wrote: > > src/master/http.cpp > > Lines 959-973 (original), 959-969 (patched) > > <https://reviews.apache.org/r/66464/diff/1/?file=1992581#file1992581line959> > > > > Hmm I think we only want to execute this block for calls which will get > > a response body (i.e., for SUBSCRIBE and RECONCILE_OPERATIONS calls). The > > other calls do not require that the 'Accept' header be set. Note that we don't require the `Accept` header to be set, if it isn't set, then the client accepts any content type, and JSON will be used. We require that the client accepts JSON or Protobuf for all Master Operator API calls: https://github.com/apache/mesos/blob/be47e96e727f07758ff9b8ba1c23bbec2a489cd6/src/master/http.cpp#L692-L701 Why should it be different for the Scheduler API? - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66464/#review200680 ------- On April 4, 2018, 5:47 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66464/ > --- > > (Updated April 4, 2018, 5:47 p.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > --- > > Implemented operation status reconciliation. > > > Diffs > - > > src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 > src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 > src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c > > > Diff: https://reviews.apache.org/r/66464/diff/1/ > > > Testing > --- > > `sudo bin/mesos-tests` on GNU/Linux > > https://reviews.apache.org/r/66468/ adds new tests. > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66464: Implemented operation status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66464/#review200746 --- src/master/http.cpp Lines 5097 (patched) <https://reviews.apache.org/r/66464/#comment281596> Do you think that we need to use `std::move` to cast it as an rvalue? It is already an rvalue per the method signature: ``` Future Master::Http::reconcileOperations( scheduler::Response::ReconcileOperations&& reconcileOperations, ContentType contentType) const ``` src/master/master.hpp Lines 2556 (patched) <https://reviews.apache.org/r/66464/#comment281597> I agree that `id` should be `const OperationID&`. Regarding returning a `Try`, what error would it return? I see this as semantically equivalent to a get method on a hashmap, so an `Option` seems to be a better fit. - Gaston Kleiman On April 4, 2018, 5:47 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66464/ > --- > > (Updated April 4, 2018, 5:47 p.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > --- > > Implemented operation status reconciliation. > > > Diffs > - > > src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 > src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 > src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c > > > Diff: https://reviews.apache.org/r/66464/diff/1/ > > > Testing > --- > > `sudo bin/mesos-tests` on GNU/Linux > > https://reviews.apache.org/r/66468/ adds new tests. > > > Thanks, > > Gaston Kleiman > >
Review Request 66489: Cleaned up internal evolve functions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66489/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- This patch removes the `mesos::` prefix from declarations where possible, uses a consistent number of empty lines in the header, and reorders some declarations. Diffs - src/internal/evolve.hpp e00ac7175438f758b10c0aa9485fc29ffa4efa29 src/internal/evolve.cpp f598ea11b64bdbface5bf1f124a02ea876196b67 Diff: https://reviews.apache.org/r/66489/diff/1/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66461: Added an evolve function for `v1::scheduler::Response`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66461/ --- (Updated April 6, 2018, 5:16 p.m.) Review request for mesos and Greg Mann. Changes --- Addressed feedback. Repository: mesos Description --- Added an evolve function for `v1::scheduler::Response`. Diffs (updated) - src/internal/evolve.hpp e00ac7175438f758b10c0aa9485fc29ffa4efa29 src/internal/evolve.cpp f598ea11b64bdbface5bf1f124a02ea876196b67 Diff: https://reviews.apache.org/r/66461/diff/2/ Changes: https://reviews.apache.org/r/66461/diff/1-2/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66461: Added an evolve function for `v1::scheduler::Response`.
> On April 6, 2018, 2:01 p.m., Greg Mann wrote: > > src/internal/evolve.hpp > > Lines 120 (patched) > > <https://reviews.apache.org/r/66461/diff/1/?file=1992542#file1992542line123> > > > > Is the `mesos::` necessary in the type of the response argument here? > > Ditto in the implementation. Nope, I removed it from this function here and cleaned up the rest of the evolve functions on this RR: https://reviews.apache.org/r/66489/ - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66461/#review200669 ------- On April 6, 2018, 5:16 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66461/ > --- > > (Updated April 6, 2018, 5:16 p.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > --- > > Added an evolve function for `v1::scheduler::Response`. > > > Diffs > - > > src/internal/evolve.hpp e00ac7175438f758b10c0aa9485fc29ffa4efa29 > src/internal/evolve.cpp f598ea11b64bdbface5bf1f124a02ea876196b67 > > > Diff: https://reviews.apache.org/r/66461/diff/2/ > > > Testing > --- > > `sudo bin/mesos-tests` on GNU/Linux > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/ --- (Updated April 6, 2018, 2:17 p.m.) Review request for mesos and Greg Mann. Changes --- Fixed typo. Repository: mesos Description --- This patch adds a `call()` method to the scheduler library that allows clients to send a `v1::scheduler::Call` to the master and receive a `v1::scheduler::Response`. It is going to be used to test operation state reconciliation. Diffs (updated) - include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 60b17b9be74132c81532d22eba681feb899b22a3 src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 Diff: https://reviews.apache.org/r/66460/diff/3/ Changes: https://reviews.apache.org/r/66460/diff/2-3/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.
> On April 6, 2018, 2:13 p.m., Vinod Kone wrote: > > include/mesos/v1/scheduler.hpp > > Line 50 (original), 54-55 (patched) > > <https://reviews.apache.org/r/66460/diff/2/?file=1993421#file1993421line54> > > > > Since we don't guarantee backwards compat for this library, can we just > > update the signature of `send`? Would the libmesos binary then be compatible? Or would frameworks then have to be recompiled using this new header in order to use new libmesos binaries? > On April 6, 2018, 2:13 p.m., Vinod Kone wrote: > > include/mesos/v1/scheduler.hpp > > Lines 112-113 (patched) > > <https://reviews.apache.org/r/66460/diff/2/?file=1993421#file1993421line112> > > > > What do you mean by `SUBSCRIBED` calls? There's is a typo there, I mean `SUBSCRIBE` calls, for which the response is a stream. - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/#review200670 ------- On April 6, 2018, 2 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66460/ > --- > > (Updated April 6, 2018, 2 p.m.) > > > Review request for mesos, Greg Mann and Vinod Kone. > > > Repository: mesos > > > Description > --- > > This patch adds a `call()` method to the scheduler library that allows > clients to send a `v1::scheduler::Call` to the master and receive a > `v1::scheduler::Response`. > > It is going to be used to test operation state reconciliation. > > > Diffs > - > > include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp > 60b17b9be74132c81532d22eba681feb899b22a3 > src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 > > > Diff: https://reviews.apache.org/r/66460/diff/2/ > > > Testing > --- > > `sudo bin/mesos-tests` on GNU/Linux > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/ --- (Updated April 6, 2018, 1:59 p.m.) Review request for mesos and Greg Mann. Changes --- Addressed Greg's feedback. Repository: mesos Description --- This patch adds a `call()` method to the scheduler library that allows clients to send a `v1::scheduler::Call` to the master and receive a `v1::scheduler::Response`. It is going to be used to test operation state reconciliation. Diffs (updated) - include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 60b17b9be74132c81532d22eba681feb899b22a3 src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 Diff: https://reviews.apache.org/r/66460/diff/2/ Changes: https://reviews.apache.org/r/66460/diff/1-2/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.
> On April 6, 2018, 1:24 p.m., Greg Mann wrote: > > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp > > Lines 143 (patched) > > <https://reviews.apache.org/r/66460/diff/1/?file=1992565#file1992565line143> > > > > What do you think about using `LOG(FATAL)` here instead of > > `UNREACHABLE()` in order to provide some useful logging before terminating? > > Might be nicer for a user than having to dig through the source to > > understand exactly what's going on. This is consistent with what we do for `reconnect()`, right above this method. Note that there are no JNI bindings for this method, so it should really be impossible to reach it =). > On April 6, 2018, 1:24 p.m., Greg Mann wrote: > > src/scheduler/scheduler.cpp > > Lines 266 (patched) > > <https://reviews.apache.org/r/66460/diff/1/?file=1992566#file1992566line266> > > > > Suggestion: I think it might improve readability slightly to use a name > > like `callMessage` instead of `call_`? Here and in the continuations below. > > Up to you. Good idea! Me likey! - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/#review200661 --- On April 4, 2018, 4:53 p.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66460/ > --- > > (Updated April 4, 2018, 4:53 p.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > --- > > This patch adds a `call()` method to the scheduler library that allows > clients to send a `v1::scheduler::Call` to the master and receive a > `v1::scheduler::Response`. > > It is going to be used to test operation state reconciliation. > > > Diffs > - > > include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp > 60b17b9be74132c81532d22eba681feb899b22a3 > src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 > > > Diff: https://reviews.apache.org/r/66460/diff/1/ > > > Testing > --- > > `sudo bin/mesos-tests` on GNU/Linux > > > Thanks, > > Gaston Kleiman > >
Review Request 66467: Added a test helper for creating `RECONCILE_OPERATIONS` v1 calls.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66467/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- Added a test helper for creating `RECONCILE_OPERATIONS` v1 calls. Diffs - src/tests/mesos.hpp 46c271b5c5bedbdabd58b3cdbb82216d55c846bd Diff: https://reviews.apache.org/r/66467/diff/1/ Testing --- `sudo bin/mesos-tests` on GNU/Linux The tests added by https://reviews.apache.org/r/66468/ use this helper. Thanks, Gaston Kleiman
Review Request 66466: Updated `RESERVE()` helper to allow specifying an operation ID.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66466/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- Updated `RESERVE()` helper to allow specifying an operation ID. Diffs - src/tests/mesos.hpp 46c271b5c5bedbdabd58b3cdbb82216d55c846bd Diff: https://reviews.apache.org/r/66466/diff/1/ Testing --- `sudo bin/mesos-tests` on GNU/Linux The tests added by https://reviews.apache.org/r/66468/ use this helper with an `OperationID`. Thanks, Gaston Kleiman
Review Request 66465: Updated `using` statements in `tests/mesos.hpp`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66465/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- Updated `using` statements in `tests/mesos.hpp`. Diffs - src/tests/mesos.hpp 46c271b5c5bedbdabd58b3cdbb82216d55c846bd Diff: https://reviews.apache.org/r/66465/diff/1/ Testing --- `sudo bin/mesos-tests` on GNU/Linux The tests added by https://reviews.apache.org/r/66468/ use these statements. Thanks, Gaston Kleiman
Review Request 66464: Implemented operation status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66464/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- Implemented operation status reconciliation. Diffs - src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c Diff: https://reviews.apache.org/r/66464/diff/1/ Testing --- `sudo bin/mesos-tests` on GNU/Linux https://reviews.apache.org/r/66468/ adds new tests. Thanks, Gaston Kleiman
Review Request 66463: Added a master metric for operations reconciliation messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66463/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- Added a master metric for operations reconciliation messages. Diffs - docs/monitoring.md 12e2103eb041e3e1b99bddafafcf4c615205fb0c docs/operator-http-api.md 10dcac83fa70da5760a5c231665d7498cc168622 src/master/metrics.hpp 5699c64bac19cb735a4c72f9a067b1338c2ac3c2 src/master/metrics.cpp 894c04173d218ffd891d203254035fe4756cc8bb src/tests/master_tests.cpp be7a3cc6126d2fe7c56c49b3da5f6f4bf29657f5 Diff: https://reviews.apache.org/r/66463/diff/1/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Review Request 66462: Added new operation states to be used for status reconciliation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66462/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- Added new operation states to be used for status reconciliation. Diffs - include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c src/slave/slave.cpp b17854788ceb63f3748380c546a13531e86f0dda Diff: https://reviews.apache.org/r/66462/diff/1/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Review Request 66461: Added an evolve function for `v1::scheduler::Response`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66461/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- Added an evolve function for `v1::scheduler::Response`. Diffs - src/internal/evolve.hpp e00ac7175438f758b10c0aa9485fc29ffa4efa29 src/internal/evolve.cpp f598ea11b64bdbface5bf1f124a02ea876196b67 Diff: https://reviews.apache.org/r/66461/diff/1/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Review Request 66460: Added a `call()` method to the v1 scheduler library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- This patch adds a `call()` method to the scheduler library that allows clients to send a `v1::scheduler::Call` to the master and receive a `v1::scheduler::Response`. It is going to be used to test operation state reconciliation. Diffs - include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 60b17b9be74132c81532d22eba681feb899b22a3 src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 Diff: https://reviews.apache.org/r/66460/diff/1/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Review Request 66459: Fixed bug in `Master::updateSlave()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66459/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- A part of `Master::updateSlave()` doesn't account for operations created via the operator API; this patch fixes that. Diffs - src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c Diff: https://reviews.apache.org/r/66459/diff/1/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Review Request 66458: Fixed handling of operations in `master::recoverFramework()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66458/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- `Master::recoverFramework()` only recovers operations affecting agent default resources. This patch makes it also recover operations affecting resources managed by resource providers. It also fixes a bug in which not just the corresponding operations, but all the ones affecting agent default resources will be added to the framework. Diffs - src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c Diff: https://reviews.apache.org/r/66458/diff/1/ Testing --- `sudo bin/mesos-tests` on GNU/Linux Thanks, Gaston Kleiman
Re: Review Request 66365: Fixed generation of operation statuses in `Slave::applyOperation()`.
> On March 30, 2018, 8:38 a.m., Greg Mann wrote: > > src/slave/slave.cpp > > Lines 4266-4268 (patched) > > <https://reviews.apache.org/r/66365/diff/1/?file=1990343#file1990343line4266> > > > > Was this formatting produced by clang-format? > > > > I would probably do: > > ``` > > Option operationId = > > message.operation_info().has_id() > > ? message.operation_info().id() > > : Option::none(); > > ``` > > but if clang-format generated this, then let's just keep it. Yeah, our `clang-format` generated it... but as discussed offline, I updated the patch to match the style of the ternary statement right above this one. - Gaston --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66365/#review200239 ------- On April 2, 2018, 10:15 a.m., Gaston Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66365/ > --- > > (Updated April 2, 2018, 10:15 a.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > --- > > This patch updates `Slave::applyOperation()` to make it include the > operation ID in operation statuses for operations that have an ID. > > > Diffs > - > > src/slave/slave.cpp a4bd4ccd3fc59c3c0e462d9b480f5424b3e52d7a > > > Diff: https://reviews.apache.org/r/66365/diff/2/ > > > Testing > --- > > Tests still pass on GNU/Linux. > > > Thanks, > > Gaston Kleiman > >
Re: Review Request 66365: Fixed generation of operation statuses in `Slave::applyOperation()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66365/ --- (Updated April 2, 2018, 10:15 a.m.) Review request for mesos and Greg Mann. Changes --- Addressed Greg's comment. Repository: mesos Description --- This patch updates `Slave::applyOperation()` to make it include the operation ID in operation statuses for operations that have an ID. Diffs (updated) - src/slave/slave.cpp a4bd4ccd3fc59c3c0e462d9b480f5424b3e52d7a Diff: https://reviews.apache.org/r/66365/diff/2/ Changes: https://reviews.apache.org/r/66365/diff/1-2/ Testing --- Tests still pass on GNU/Linux. Thanks, Gaston Kleiman
Review Request 66367: Made the master drop `RECONCILE_OPERATIONS` calls from v0 schedulers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66367/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- Made the master drop `RECONCILE_OPERATIONS` calls from v0 schedulers. Diffs - src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 Diff: https://reviews.apache.org/r/66367/diff/1/ Testing --- Existing tests pass on GNU/Linux. Thanks, Gaston Kleiman
Review Request 66366: Fixed generation of operation status updates in `Master::_apply()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66366/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- This patch updates `Master::_apply() ` to make it include operation ID in the initial operation status of operations that have an ID. Diffs - src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 Diff: https://reviews.apache.org/r/66366/diff/1/ Testing --- Tests still pass on GNU/Linux. Thanks, Gaston Kleiman
