Re: Review Request 39450: Quota: Added hierarchical allocator-specific tests.
> On Nov. 10, 2015, 4:03 p.m., Joerg Schad wrote: > > src/tests/hierarchical_allocator_tests.cpp, line 1151 > > <https://reviews.apache.org/r/39450/diff/6/?file=1113990#file1113990line1151> > > > > Also the simple case of correct behavior with several frameworks in one > > role would be interesting. DO you mean something like `MultipleFrameworksInRoleWithQuota`, L1370? > On Nov. 10, 2015, 4:03 p.m., Joerg Schad wrote: > > src/tests/hierarchical_allocator_tests.cpp, line 1174 > > <https://reviews.apache.org/r/39450/diff/6/?file=1113990#file1113990line1174> > > > > Does it make sense to refelect that in the role name? Good idea! > On Nov. 10, 2015, 4:03 p.m., Joerg Schad wrote: > > src/tests/hierarchical_allocator_tests.cpp, line 1619 > > <https://reviews.apache.org/r/39450/diff/6/?file=1113990#file1113990line1619> > > > > This describes more the user story, should we maybe focus on what the > > test itself intends to test? I'd say, the test intends to test exactly this user story : ). For me it's easier to understand the purpose of the test if I know the background or the issue. Because this test is not trivial, I would argue the user story helps. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39450/#review105885 --- On Nov. 23, 2015, 10:45 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39450/ > --- > > (Updated Nov. 23, 2015, 10:45 a.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, > and Joseph Wu. > > > Bugs: MESOS-3984 > https://issues.apache.org/jira/browse/MESOS-3984 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/tests/hierarchical_allocator_tests.cpp > 740cfa801ee90417c038308192d1f4f2416f8315 > > Diff: https://reviews.apache.org/r/39450/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 39450: Quota: Added hierarchical allocator-specific tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39450/ --- (Updated Nov. 23, 2015, 4:11 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, and Joseph Wu. Changes --- Rebased. Bugs: MESOS-3984 https://issues.apache.org/jira/browse/MESOS-3984 Repository: mesos Description --- See summary. Diffs (updated) - src/tests/hierarchical_allocator_tests.cpp 740cfa801ee90417c038308192d1f4f2416f8315 Diff: https://reviews.apache.org/r/39450/diff/ Testing --- make check (Mac OS X 10.10.4) Thanks, Alexander Rukletsov
Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40332/ --- (Updated Nov. 23, 2015, 4:11 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, Joseph Wu, and Qian Zhang. Changes --- Rebased. Bugs: MESOS-3981 https://issues.apache.org/jira/browse/MESOS-3981 Repository: mesos Description --- See summary. Diffs (updated) - src/master/allocator/mesos/hierarchical.hpp c65fe35198b846da2dc959dd467a21ff6edd30a9 src/master/allocator/mesos/hierarchical.cpp 2765526047767cbd19d13c11ecfa6e90c505b3a7 Diff: https://reviews.apache.org/r/40332/diff/ Testing --- make check (Mac OS X 10.10.4) Thanks, Alexander Rukletsov
Re: Review Request 39400: Quota: Implemented quota API.
> On Nov. 23, 2015, 8:07 a.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 391
> > <https://reviews.apache.org/r/39400/diff/7/?file=1123269#file1123269line391>
> >
> > For this newly added member `quotaRoleSorter`, I think we also need to
> > initialize it in the constructor like what we did for `roleSorter`.
> > ```
> > HierarchicalAllocatorProcess(
> > const std::function<Sorter*()>& _roleSorterFactory,
> > const std::function<Sorter*()>& _frameworkSorterFactory)
> > : ProcessBase(process::ID::generate("hierarchical-allocator")),
> > initialized(false),
> > metrics(*this),
> > roleSorterFactory(_roleSorterFactory),
> > frameworkSorterFactory(_frameworkSorterFactory),
> > roleSorter(NULL) {}<--- we initialized roleSorter here.
> > ```
Let's fix it in https://reviews.apache.org/r/40586/
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39400/#review107563
---
On Nov. 11, 2015, 10:29 a.m., Alexander Rukletsov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39400/
> ---
>
> (Updated Nov. 11, 2015, 10:29 a.m.)
>
>
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere,
> and Joseph Wu.
>
>
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
>
>
> Repository: mesos
>
>
> Description
> ---
>
> See summary.
>
>
> Diffs
> -
>
> src/master/allocator/mesos/hierarchical.hpp
> cd0d3e0bd234fcf4db9af2e376ea311937204f75
> src/master/allocator/mesos/hierarchical.cpp
> 14fef63714721fcda7cea3c28704766efda6d007
>
> Diff: https://reviews.apache.org/r/39400/diff/
>
>
> Testing
> ---
>
> make check (Mac OS X 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 40586: Corrected typos and a formatting issue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40586/ --- (Updated Nov. 23, 2015, 5:38 p.m.) Review request for mesos and Joris Van Remoortere. Changes --- Corrected one more typo. Summary (updated) - Corrected typos and a formatting issue. Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/quota/quota.hpp 8276749efbca167c56e337aa9155fb7db739c1c9 src/master/allocator/mesos/hierarchical.hpp c65fe35198b846da2dc959dd467a21ff6edd30a9 Diff: https://reviews.apache.org/r/40586/diff/ Testing --- make check (Mac OS X 10.10.4) Thanks, Alexander Rukletsov
Re: Review Request 40586: Corrected a typo and a formatting issue.
> On Nov. 23, 2015, 4:35 a.m., Guangya Liu wrote: > > src/master/allocator/mesos/hierarchical.hpp, lines 367-377 > > <https://reviews.apache.org/r/40586/diff/1/?file=1135512#file1135512line367> > > > > Sorry, what is the update for this file? removed extra blank line - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40586/#review107548 --- On Nov. 22, 2015, 11:59 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40586/ > --- > > (Updated Nov. 22, 2015, 11:59 p.m.) > > > Review request for mesos and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > include/mesos/quota/quota.hpp 8276749efbca167c56e337aa9155fb7db739c1c9 > src/master/allocator/mesos/hierarchical.hpp > c65fe35198b846da2dc959dd467a21ff6edd30a9 > > Diff: https://reviews.apache.org/r/40586/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 39320: Speeded up the test by reducing the allocation timeout.
> On Nov. 24, 2015, 1:52 p.m., Benjamin Bannier wrote: > > src/tests/fault_tolerance_tests.cpp, line 778 > > <https://reviews.apache.org/r/39320/diff/3/?file=1138778#file1138778line778> > > > > You should just use `master::Flags().allocation_interval` to simplify > > this. Do you think it will be cleaner? I have following thoughts: - it extracts the default value from the `master::Flags` structs instead of being in sync with the test environment created by `CreateMasterFlags()`; - we create a complete `master::Flags` object in order to use one flag. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39320/#review107780 --- On Nov. 24, 2015, 1:13 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39320/ > --- > > (Updated Nov. 24, 2015, 1:13 p.m.) > > > Review request for mesos, Benjamin Bannier, Bernd Mathiske, Neil Conway, and > Till Toenshoff. > > > Bugs: MESOS-3732 > https://issues.apache.org/jira/browse/MESOS-3732 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/tests/fault_tolerance_tests.cpp > 3bbd6cbf6b101415c90991c48f00f33a5aff5590 > src/tests/mesos.hpp a2a76f55f9a4091cccfccd9672c2d2897b34ddcd > src/tests/mesos.cpp b2211d59ed571ec8ddc132f69ed1cf5da9eeef10 > > Diff: https://reviews.apache.org/r/39320/diff/ > > > Testing > --- > > make check > > The run time for the `FaultToleranceTest.FrameworkReregister` test has > reduced from more than `1s` to less than `100ms`. > Before: > ``` > [ RUN ] FaultToleranceTest.FrameworkReregister > [ OK ] FaultToleranceTest.FrameworkReregister (1018 ms) > ``` > After: > ``` > [ RUN ] FaultToleranceTest.FrameworkReregister > [ OK ] FaultToleranceTest.FrameworkReregister (49 ms) > ``` > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40332/ --- (Updated Nov. 24, 2015, 3:07 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, Joseph Wu, and Qian Zhang. Changes --- Rephrased logs. Bugs: MESOS-3981 https://issues.apache.org/jira/browse/MESOS-3981 Repository: mesos Description --- See summary. Diffs (updated) - src/master/allocator/mesos/hierarchical.hpp 2a21364fdcaa4ec5e5567b9f367c14a1579b9a49 src/master/allocator/mesos/hierarchical.cpp aee8ced1fbfec8cf30cb939ff67fadfc6b08f37a Diff: https://reviews.apache.org/r/40332/diff/ Testing --- make check (Mac OS X 10.10.4) Thanks, Alexander Rukletsov
Re: Review Request 39320: Speeded up the test by reducing the allocation timeout.
> On Nov. 23, 2015, 6:45 p.m., Neil Conway wrote: > > An alternative would be to pause and advance the clock by the > > allocation_interval. Do you think that would be worth doing instead? That's correct. I was not sure what approach is easier to understand. After you input I have checked the "testing-patterns.md" doc and it looks like clock magic is well documented and may therefore be a preferred solution. Will update the patch shortly. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39320/#review107625 --- On Nov. 23, 2015, 5:52 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39320/ > --- > > (Updated Nov. 23, 2015, 5:52 p.m.) > > > Review request for mesos, Benjamin Bannier, Bernd Mathiske, and Till > Toenshoff. > > > Bugs: MESOS-3732 > https://issues.apache.org/jira/browse/MESOS-3732 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/tests/fault_tolerance_tests.cpp > 3bbd6cbf6b101415c90991c48f00f33a5aff5590 > > Diff: https://reviews.apache.org/r/39320/diff/ > > > Testing > --- > > make check > > The run time for the `FaultToleranceTest.FrameworkReregister` test has > reduced from more than `1s` to less than `100ms`. > Before: > ``` > [ RUN ] FaultToleranceTest.FrameworkReregister > [ OK ] FaultToleranceTest.FrameworkReregister (1018 ms) > ``` > After: > ``` > [ RUN ] FaultToleranceTest.FrameworkReregister > [ OK ] FaultToleranceTest.FrameworkReregister (49 ms) > ``` > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 39320: Speeded up the test by reducing the allocation timeout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39320/ --- (Updated Nov. 24, 2015, 1:13 p.m.) Review request for mesos, Benjamin Bannier, Bernd Mathiske, Neil Conway, and Till Toenshoff. Changes --- Alternative approach to speeding up the test. Bugs: MESOS-3732 https://issues.apache.org/jira/browse/MESOS-3732 Repository: mesos Description --- See summary. Diffs (updated) - src/tests/fault_tolerance_tests.cpp 3bbd6cbf6b101415c90991c48f00f33a5aff5590 src/tests/mesos.hpp a2a76f55f9a4091cccfccd9672c2d2897b34ddcd src/tests/mesos.cpp b2211d59ed571ec8ddc132f69ed1cf5da9eeef10 Diff: https://reviews.apache.org/r/39320/diff/ Testing --- make check The run time for the `FaultToleranceTest.FrameworkReregister` test has reduced from more than `1s` to less than `100ms`. Before: ``` [ RUN ] FaultToleranceTest.FrameworkReregister [ OK ] FaultToleranceTest.FrameworkReregister (1018 ms) ``` After: ``` [ RUN ] FaultToleranceTest.FrameworkReregister [ OK ] FaultToleranceTest.FrameworkReregister (49 ms) ``` Thanks, Alexander Rukletsov
Re: Review Request 39320: Speeded up the test by reducing the allocation timeout.
> On Nov. 23, 2015, 6:45 p.m., Neil Conway wrote: > > An alternative would be to pause and advance the clock by the > > allocation_interval. Do you think that would be worth doing instead? > > Alexander Rukletsov wrote: > That's correct. I was not sure what approach is easier to understand. > After you input I have checked the "testing-patterns.md" doc and it looks > like clock magic is well documented and may therefore be a preferred > solution. Will update the patch shortly. Please check the updated version, slightly more intrusive. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39320/#review107625 --- On Nov. 24, 2015, 1:13 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39320/ > --- > > (Updated Nov. 24, 2015, 1:13 p.m.) > > > Review request for mesos, Benjamin Bannier, Bernd Mathiske, Neil Conway, and > Till Toenshoff. > > > Bugs: MESOS-3732 > https://issues.apache.org/jira/browse/MESOS-3732 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/tests/fault_tolerance_tests.cpp > 3bbd6cbf6b101415c90991c48f00f33a5aff5590 > src/tests/mesos.hpp a2a76f55f9a4091cccfccd9672c2d2897b34ddcd > src/tests/mesos.cpp b2211d59ed571ec8ddc132f69ed1cf5da9eeef10 > > Diff: https://reviews.apache.org/r/39320/diff/ > > > Testing > --- > > make check > > The run time for the `FaultToleranceTest.FrameworkReregister` test has > reduced from more than `1s` to less than `100ms`. > Before: > ``` > [ RUN ] FaultToleranceTest.FrameworkReregister > [ OK ] FaultToleranceTest.FrameworkReregister (1018 ms) > ``` > After: > ``` > [ RUN ] FaultToleranceTest.FrameworkReregister > [ OK ] FaultToleranceTest.FrameworkReregister (49 ms) > ``` > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 40351: Quota: Added rescinding offers for set quota requests.
> On Nov. 23, 2015, 3:35 a.m., Guangya Liu wrote: > > src/master/quota_handler.cpp, line 332 > > <https://reviews.apache.org/r/40351/diff/6/?file=1135522#file1135522line332> > > > > s/enough/as many as? > > > > I'm proposing this because we cannot guarantee there are enough > > outstanding offers to satisify the quota request, comments? Good point, will reword. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40351/#review107545 --- On Nov. 23, 2015, 12:57 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40351/ > --- > > (Updated Nov. 23, 2015, 12:57 a.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, > Joseph Wu, and Qian Zhang. > > > Bugs: MESOS-3912 > https://issues.apache.org/jira/browse/MESOS-3912 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/master.hpp d4b1edde98925fd51e056f253758afea779be9ed > src/master/quota_handler.cpp 86d7331aa79adb1d9a3009552fc4c2aed0229804 > > Diff: https://reviews.apache.org/r/40351/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 39492: Added status endpoint for quota master endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39492/#review107185 --- src/master/quota_handler.cpp (line 347) <https://reviews.apache.org/r/39492/#comment166184> let's `static_cast` to `int` explicitly - Alexander Rukletsov On Nov. 19, 2015, 3:19 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39492/ > --- > > (Updated Nov. 19, 2015, 3:19 p.m.) > > > Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van > Remoortere. > > > Bugs: MESOS-3073 > https://issues.apache.org/jira/browse/MESOS-3073 > > > Repository: mesos > > > Description > --- > > Added status handling for quota master endpoint. > > > Diffs > - > > include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d > src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 > src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f > > Diff: https://reviews.apache.org/r/39492/diff/ > > > Testing > --- > > Tests are in next Review. > > > Thanks, > > Joerg Schad > >
Re: Review Request 39614: Quota: Added Status Validation Tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39614/#review107181 --- src/tests/master_quota_tests.cpp (line 435) <https://reviews.apache.org/r/39614/#comment166178> let's wrap "from" to avoid jaggedness. src/tests/master_quota_tests.cpp (line 443) <https://reviews.apache.org/r/39614/#comment166181> Don't we need to authenticate? src/tests/master_quota_tests.cpp (lines 455 - 456) <https://reviews.apache.org/r/39614/#comment166177> It feels like this test may not test what it should. The field may miss for two reasons: no quotas set _or_ there is no such field at all : ). I'd suggest to either convert to the protobuf and then check the size of the collection, or check against entire `JSON` object. src/tests/master_quota_tests.cpp (line 479) <https://reviews.apache.org/r/39614/#comment166171> indent src/tests/master_quota_tests.cpp (lines 479 - 480) <https://reviews.apache.org/r/39614/#comment166172> blank line src/tests/master_quota_tests.cpp (line 490) <https://reviews.apache.org/r/39614/#comment166170> space src/tests/master_quota_tests.cpp (lines 490 - 491) <https://reviews.apache.org/r/39614/#comment166173> blank line src/tests/master_quota_tests.cpp (lines 494 - 497) <https://reviews.apache.org/r/39614/#comment166179> Let's add a check that total cluster resources contain `quotaResources`. src/tests/master_quota_tests.cpp (lines 495 - 496) <https://reviews.apache.org/r/39614/#comment166174> Fits one line src/tests/master_quota_tests.cpp (line 496) <https://reviews.apache.org/r/39614/#comment166175> Please use constant `ROLE1` src/tests/master_quota_tests.cpp (lines 500 - 503) <https://reviews.apache.org/r/39614/#comment166180> indentation src/tests/master_quota_tests.cpp (line 520) <https://reviews.apache.org/r/39614/#comment166182> How about "Convert `JSON` response to `QuotaStatus` protobuf"? Also, mind backticks. src/tests/master_quota_tests.cpp (line 521) <https://reviews.apache.org/r/39614/#comment166183> `protobuf::parse()` does not compile? src/tests/master_quota_tests.cpp (line 524) <https://reviews.apache.org/r/39614/#comment166169> `RepeatedField::size()` returns `int` - Alexander Rukletsov On Nov. 19, 2015, 2:48 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39614/ > ------- > > (Updated Nov. 19, 2015, 2:48 p.m.) > > > Review request for mesos, Alexander Rukletsov and Bernd Mathiske. > > > Bugs: MESOS-3073 > https://issues.apache.org/jira/browse/MESOS-3073 > > > Repository: mesos > > > Description > --- > > Quota: Added Status Validation Tests. > > > Diffs > - > > src/tests/master_quota_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/39614/diff/ > > > Testing > --- > > make check > > > Thanks, > > Joerg Schad > >
Re: Review Request 39492: Added status endpoint for quota master endpoint.
> On Nov. 19, 2015, 2:53 p.m., Alexander Rukletsov wrote: > > src/master/quota_handler.cpp, line 338 > > <https://reviews.apache.org/r/39492/diff/4/?file=1131960#file1131960line338> > > > > s/Status/status > > Do you think it makes sense to add `request.body`. > > Joerg Schad wrote: > Considered adding request body, but as of right now the body is empty, or? I'm thinking about the evolution of the request. Maybe it makes sense to add URL parameters? Will people be using extra flags? On the other side we can add it later. Dropping the issue. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39492/#review107176 --- On Nov. 19, 2015, 3:19 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39492/ > --- > > (Updated Nov. 19, 2015, 3:19 p.m.) > > > Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van > Remoortere. > > > Bugs: MESOS-3073 > https://issues.apache.org/jira/browse/MESOS-3073 > > > Repository: mesos > > > Description > --- > > Added status handling for quota master endpoint. > > > Diffs > - > > include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d > src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 > src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f > > Diff: https://reviews.apache.org/r/39492/diff/ > > > Testing > --- > > Tests are in next Review. > > > Thanks, > > Joerg Schad > >
Re: Review Request 39614: Quota: Added Status Validation Tests.
> On Nov. 6, 2015, 1:13 a.m., Joseph Wu wrote: > > src/tests/master_quota_tests.cpp, lines 732-734 > > <https://reviews.apache.org/r/39614/diff/2/?file=1106413#file1106413line732> > > > > Have you considered merging this test with > > `AvailableResourcesMultipleAgents`? > > Joerg Schad wrote: > In my opinion it is good to test both aspects seperatly from each other. I think it's okay to merge as long as it's clear what fails. Same for situations when no quota is set. Moreover, once we have a test with several quotas, we can check status of multiple quotas as well. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39614/#review105343 --- On Nov. 19, 2015, 2:48 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39614/ > --- > > (Updated Nov. 19, 2015, 2:48 p.m.) > > > Review request for mesos, Alexander Rukletsov and Bernd Mathiske. > > > Bugs: MESOS-3073 > https://issues.apache.org/jira/browse/MESOS-3073 > > > Repository: mesos > > > Description > --- > > Quota: Added Status Validation Tests. > > > Diffs > - > > src/tests/master_quota_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/39614/diff/ > > > Testing > --- > > make check > > > Thanks, > > Joerg Schad > >
Re: Review Request 40351: Quota: Added rescinding offers for set quota requests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40351/ --- (Updated Nov. 19, 2015, 5:01 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, and Joseph Wu. Changes --- Added a missing blank line. Bugs: MESOS-3912 https://issues.apache.org/jira/browse/MESOS-3912 Repository: mesos Description --- See summary. Diffs (updated) - src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f Diff: https://reviews.apache.org/r/40351/diff/ Testing --- make check (Mac OS X 10.10.4) Thanks, Alexander Rukletsov
Re: Review Request 40351: Quota: Added rescinding offers for set quota requests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40351/ --- (Updated Nov. 19, 2015, 5:10 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, and Joseph Wu. Changes --- Fixed a typo. Bugs: MESOS-3912 https://issues.apache.org/jira/browse/MESOS-3912 Repository: mesos Description --- See summary. Diffs (updated) - src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f Diff: https://reviews.apache.org/r/40351/diff/ Testing --- make check (Mac OS X 10.10.4) Thanks, Alexander Rukletsov
Re: Review Request 40351: Quota: Added rescinding offers for set quota requests.
> On Nov. 17, 2015, 6:16 a.m., Qian Zhang wrote: > > src/master/quota_handler.cpp, line 211 > > <https://reviews.apache.org/r/40351/diff/2/?file=1126317#file1126317line211> > > > > For ```offer->resources()```, before doing the math, do we need to call > > ```flatten()``` to remove role too? For dynamic reservation, in > > ```Http::_operation()```, I see we do not call ```flatten()``` for offered > > resources too, is it a bug? > > Alexander Rukletsov wrote: > In this case everything should be fine. IIUC, there is only one reason > why `offer->resources()` has non '*' role: it's a statically reserved > resource. Quota is orthogonal to static reservations, hence we should not > rescind those offers. > > However, I think this check should be removed here. We may rescind more > offers than necessary to satisfy remaining resources (because we want to > rescind from a certain number of agents). I'll think about it. > > Qian Zhang wrote: > But I think for the dynamic reserved resource, ```offer->resources()``` > also has non * role and also has non-empty ```ReservationInfo```, right? Here is "cheat sheet" from @MPark: ``` has_reservation && role == "*" : invalid has_reservation && role != "*": dynamically reserved !has_reservation && role == "*": unreserved !has_reservation && role != "*": statically reserved ``` According to it, you're right. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40351/#review106799 --- On Nov. 19, 2015, 5:10 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40351/ > --- > > (Updated Nov. 19, 2015, 5:10 p.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, > and Joseph Wu. > > > Bugs: MESOS-3912 > https://issues.apache.org/jira/browse/MESOS-3912 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 > src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f > > Diff: https://reviews.apache.org/r/40351/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 40399: Quota: Introduced quota registry operations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40399/ --- (Updated Nov. 19, 2015, 4:45 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, and Joseph Wu. Changes --- Simplified protobuf manipulation code. Bugs: MESOS-3165 https://issues.apache.org/jira/browse/MESOS-3165 Repository: mesos Description --- See summary. Diffs (updated) - src/master/quota.hpp 8fc984003762a339a796b7a516362e21dab0a6e5 src/master/quota.cpp 835b191460d86e912ffdac8160459b4a0643bd88 Diff: https://reviews.apache.org/r/40399/diff/ Testing --- make check (Mac OS X 10.10.4) Thanks, Alexander Rukletsov
Re: Review Request 40351: Quota: Added rescinding offers for set quota requests.
> On Nov. 18, 2015, 7:51 a.m., Qian Zhang wrote: > > src/master/quota_handler.cpp, line 180 > > <https://reviews.apache.org/r/40351/diff/3/?file=1128793#file1128793line180> > > > > Why do we want to rescind the offeres that do not contribute to > > satisfying quota request? Because we may rescind more than necessary to satisfy quota request (remember minimal agent count). If we have a check in place, this will effectively prevent us from doing so. Does it make sense to you? - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40351/#review106977 --- On Nov. 19, 2015, 5:10 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40351/ > --- > > (Updated Nov. 19, 2015, 5:10 p.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, > and Joseph Wu. > > > Bugs: MESOS-3912 > https://issues.apache.org/jira/browse/MESOS-3912 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 > src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f > > Diff: https://reviews.apache.org/r/40351/diff/ > > > Testing > --- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 39614: Quota: Added Status Validation Tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39614/#review107169 --- src/tests/master_quota_tests.cpp (line 724) <https://reviews.apache.org/r/39614/#comment166150> I can't understand, why `quota` and not `guarantees`. What am I missing? src/tests/master_quota_tests.cpp (line 794) <https://reviews.apache.org/r/39614/#comment166151> Let's expand this test beyond a simple size check. - Alexander Rukletsov On Oct. 24, 2015, 7:40 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39614/ > --- > > (Updated Oct. 24, 2015, 7:40 p.m.) > > > Review request for mesos, Alexander Rukletsov and Bernd Mathiske. > > > Bugs: MESOS-3073 > https://issues.apache.org/jira/browse/MESOS-3073 > > > Repository: mesos > > > Description > --- > > Quota: Added Status Validation Tests. > > > Diffs > - > > src/tests/master_quota_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/39614/diff/ > > > Testing > --- > > make check > > > Thanks, > > Joerg Schad > >
Re: Review Request 39614: Quota: Added Status Validation Tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39614/#review107159 --- src/tests/master_quota_tests.cpp (line 701) <https://reviews.apache.org/r/39614/#comment166129> Why do you call them status validation tests? Do they belong to this test section? src/tests/master_quota_tests.cpp (line 703) <https://reviews.apache.org/r/39614/#comment166130> 3rd person singular, please. src/tests/master_quota_tests.cpp (lines 703 - 705) <https://reviews.apache.org/r/39614/#comment166131> I'd say "empty quota" is a bit ambiguous. How about `StatusNoQuotas`? src/tests/master_quota_tests.cpp (line 712) <https://reviews.apache.org/r/39614/#comment166134> Do you want to check status here as well? src/tests/master_quota_tests.cpp (lines 785 - 787) <https://reviews.apache.org/r/39614/#comment166138> If we create an implicit contract that the status JSON is based on `QuotaStatus` proto—which I think is a good idea—we should maybe check we can convert the `JSON` to proto. - Alexander Rukletsov On Oct. 24, 2015, 7:40 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39614/ > --- > > (Updated Oct. 24, 2015, 7:40 p.m.) > > > Review request for mesos, Alexander Rukletsov and Bernd Mathiske. > > > Bugs: MESOS-3073 > https://issues.apache.org/jira/browse/MESOS-3073 > > > Repository: mesos > > > Description > --- > > Quota: Added Status Validation Tests. > > > Diffs > - > > src/tests/master_quota_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/39614/diff/ > > > Testing > --- > > make check > > > Thanks, > > Joerg Schad > >
Re: Review Request 39492: Added status endpoint for quota master endpoint.
> On Nov. 19, 2015, 10:39 a.m., Alexander Rukletsov wrote: > > src/master/quota_handler.cpp, line 195 > > <https://reviews.apache.org/r/39492/diff/3/?file=1126166#file1126166line195> > > > > Am I right that there is another way to achieve the same: > > ``` > > status.mutable_guarantees()->Add()->CopyFrom(quotaInfo.info); > > ``` > > > > Do you know what the difference is and what approach is preferable? > > Joerg Schad wrote: > Afaik they are equivalent. I would use Add() in cases where have the > object anyhow and do not have to get it via mutable_X(). See for example in > resources.cpp "resources.Add()->CopyFrom(that)". In cases where I only have > access to a sorrounding object (i.e status in this case) I use add_x(). Totally makes sense to me. Thanks! - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39492/#review107157 --- On Nov. 19, 2015, 12:44 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39492/ > --- > > (Updated Nov. 19, 2015, 12:44 p.m.) > > > Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van > Remoortere. > > > Bugs: MESOS-3073 > https://issues.apache.org/jira/browse/MESOS-3073 > > > Repository: mesos > > > Description > --- > > Added status handling for quota master endpoint. > > > Diffs > - > > include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d > src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 > src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f > > Diff: https://reviews.apache.org/r/39492/diff/ > > > Testing > --- > > Tests are in next Review. > > > Thanks, > > Joerg Schad > >
Re: Review Request 41593: Added `jsonify` function to stout.
> On Jan. 7, 2016, 10:25 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp, lines 97-99
> > <https://reviews.apache.org/r/41593/diff/14/?file=1181766#file1181766line97>
> >
> > Why this behaviour? I would expect if `set()` is not called, nothing is
> > printed. If a field is optional, I can imagine some cases treating the
> > absence of a value differently to `false`, effectively implementing tribool
> > logic.
> >
> > Or am I understanding the workflow wrong?
>
> Michael Park wrote:
> This behavior is consistent with other writers such as `ArrayWriter`
> where not calling `append` results in `[]`. The provided expectation is that
> a writer will always write something. If a field is optional and therefore we
> want to omit writing under certain conditions, the pattern is to not invoke
> the `write` at all.
>
> For exmaple,
> ```cpp
> 190 // Omit pid for http frameworks.
> 191 if (framework.pid.isSome()) {
> 192 writer->field("pid", string(framework.pid.get()));
> 193 }
> ```
>
> Does this seem reasonable?
I see. First, almost no-one will use `BooleanWriter` directly, but rather
indirectly via `JSON::ObjectWriter::field()`. Second the idea is once we create
an object, we should add an entry in the JSON result. No entry — no object.
Makes sense, since JSON does not allow empty values for fields AFAIK. Mind
extending the example you have in the README with this approach for optional
fields?
> On Jan. 7, 2016, 10:25 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/Makefile.am, line 38
> > <https://reviews.apache.org/r/41593/diff/14/?file=1181763#file1181763line38>
> >
> > Is formatting off or it's just review board?
>
> Michael Park wrote:
> The formatting is off I think. I can't be sure with all of these tabs all
> over the place.
Yeah, it's a bit odd. In makefiles we align using tabs.
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41593/#review113230
---
On Jan. 8, 2016, 4:47 a.m., Michael Park wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41593/
> ---
>
> (Updated Jan. 8, 2016, 4:47 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> See summary.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am
> 5ab7bc4966fe32eaddd573a4dbfd997f98b5d481
> 3rdparty/libprocess/3rdparty/stout/README.md
> a2a3dc6120b10c8fe82088b9f0ebecfa5642f945
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am
> b2dea9b93adfa3ea0415a0b5c81a369dd29b6cfe
> 3rdparty/libprocess/3rdparty/stout/include/stout/jsonify.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp
> 98ea47794b3a7c99b3cbd2418ba6e36eb5951259
> 3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt
> 62ad461eb228b688f1ceac16cfb003561ed5a806
> 3rdparty/libprocess/3rdparty/stout/tests/jsonify_tests.cpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp
> bf2a2b8a9f67c6a4cf66b156b9c14fae015a8af0
>
> Diff: https://reviews.apache.org/r/41593/diff/
>
>
> Testing
> ---
>
> Added tests to ``
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 42221: Removed references to wDRF from allocator.
> On Jan. 13, 2016, 1:52 a.m., Guangya Liu wrote: > > src/master/allocator/mesos/hierarchical.cpp, line 1185 > > <https://reviews.apache.org/r/42221/diff/1/?file=1194999#file1194999line1185> > > > > s/framwork/frameworks Why do you want plural here? - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42221/#review114126 --- On Jan. 12, 2016, 11:06 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42221/ > --- > > (Updated Jan. 12, 2016, 11:06 p.m.) > > > Review request for mesos, Ben Mahler and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > Since wDRF is encapsulated in the choice of sorter, it should not be > mentioned in the allocator code. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f > > Diff: https://reviews.apache.org/r/42221/diff/ > > > Testing > --- > > `make check` on Mac OS 10.10.4 > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 42221: Removed references to wDRF from allocator.
> On Jan. 13, 2016, 1:52 a.m., Guangya Liu wrote: > > Since the comments are mainly to developers, the `second stage` is not very > > clear to developers, but the `WDRF` seems more clear. If want to update, > > what about "fair share stage"? The fairness of the "second" stage is determined by sorters as well. For example, one may write a sorter that favors roles starting with "alex-" : ). What allocator knows is that there are two stages: the first where only quota'ed frameworks are processed and the second for all frameworks. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42221/#review114126 --- On Jan. 12, 2016, 11:06 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42221/ > --- > > (Updated Jan. 12, 2016, 11:06 p.m.) > > > Review request for mesos, Ben Mahler and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > Since wDRF is encapsulated in the choice of sorter, it should not be > mentioned in the allocator code. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f > > Diff: https://reviews.apache.org/r/42221/diff/ > > > Testing > --- > > `make check` on Mac OS 10.10.4 > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 42040: Added Quota Operator Documentation.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42040/#review113916
---
Ship it!
docs/quota.md (line 61)
<https://reviews.apache.org/r/42040/#comment174694>
Unnecessary blank line.
docs/quota.md (line 73)
<https://reviews.apache.org/r/42040/#comment174700>
I think you can kill "currently" here, you have already said that in the
previous sentence.
docs/quota.md (line 83)
<https://reviews.apache.org/r/42040/#comment174702>
Kill "HTTP" here.
docs/quota.md (line 116)
<https://reviews.apache.org/r/42040/#comment174695>
Unnecessary blank line.
docs/quota.md (line 122)
<https://reviews.apache.org/r/42040/#comment174696>
Unnecessary blank line.
docs/quota.md (line 124)
<https://reviews.apache.org/r/42040/#comment174703>
We refer to this guy as an operator. Please correct all instances.
docs/quota.md (line 155)
<https://reviews.apache.org/r/42040/#comment174704>
Kill "HTTP" here.
docs/quota.md (line 221)
<https://reviews.apache.org/r/42040/#comment174699>
You already have an anchor named "removeRequest". Please correct the naming
scheme and update all references accordingly.
docs/quota.md (line 259)
<https://reviews.apache.org/r/42040/#comment174697>
Unnecessary blank line.
docs/quota.md (line 261)
<https://reviews.apache.org/r/42040/#comment174707>
s/remaing/remaining
docs/quota.md (line 298)
<https://reviews.apache.org/r/42040/#comment174706>
Please use em-dashes ("---") without spaces instead.
docs/quota.md (line 313)
<https://reviews.apache.org/r/42040/#comment174698>
Is there a trailing blank line?
- Alexander Rukletsov
On Jan. 11, 2016, 6:51 p.m., Joerg Schad wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42040/
> -------
>
> (Updated Jan. 11, 2016, 6:51 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, Joris Van
> Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3877
> https://issues.apache.org/jira/browse/MESOS-3877
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Added Quota Operator Documentation.
>
>
> Diffs
> -
>
> docs/home.md 6f0f4b9cb9d0da1f9960ebe7f36ce186c1317535
> docs/quota.md PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42040/diff/
>
>
> Testing
> ---
>
> Rendered version: https://gist.github.com/joerg84/a2c32e25d91e33045b56
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 41769: Made allocator traverse all roles for quota allocation.
> On Jan. 13, 2016, 11:41 a.m., Alexander Rukletsov wrote: > > src/master/allocator/mesos/hierarchical.cpp, lines 1164-1166 > > <https://reviews.apache.org/r/41769/diff/2/?file=1180221#file1180221line1164> > > > > This looks like a bug to me. I can't remember why we have `break` here > > in the first place. Could you please elaborate why do you think `break` is > > fine here? > > Guangya Liu wrote: > It is a bit tricky here so I add some comments here even though I prefer > we use `continue` here. > > The reason that it is OK to use `break` is because all of the quota roles > are sorted based on starvation, if the roles in quota role sorter is > satisfied, we can assume that all roles behind current satisfied role is also > satisfied, that's why `break` here. Comments? I'm not sure it's right. Roles are sorted according to their allocation, hence a role with quota 5 and allocation 5 will come before role with quota 50 and allocation 10. I believe `continue` should be used here. Also imagine a custom sorter is used, which does not guarantee any ordering. Anyway, I've checked all the tests we have and it seems there is no test where we set quotas for multiple roles. Do you want to add such test or expand an existing one? Also we may want to add some tests around implicit roles and cover the other issue you fix in this test. There is a ticket for this already: MESOS-4292. We have to figure out what exactly we want to test and sync with NeilC. I'd be happy to help out and review, this looks like a bug! Could you please also keep Joris and Qian Zhang in the loop? Thanks! - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41769/#review114184 --- On Dec. 31, 2015, 11:34 p.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41769/ > --- > > (Updated Dec. 31, 2015, 11:34 p.m.) > > > Review request for mesos, Alexander Rukletsov, Klaus Ma, and Neil Conway. > > > Repository: mesos > > > Description > --- > > Made allocator traverse all roles for quota allocation. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > 7f900c4e024485704d79e57ae22407557598fe6c > > Diff: https://reviews.apache.org/r/41769/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Guangya Liu > >
Re: Review Request 41769: Made allocator traverse all roles for quota allocation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41769/#review114184 --- src/master/allocator/mesos/hierarchical.cpp (lines 1164 - 1166) <https://reviews.apache.org/r/41769/#comment175008> This looks like a bug to me. I can't remember why we have `break` here in the first place. Could you please elaborate why do you think `break` is fine here? - Alexander Rukletsov On Dec. 31, 2015, 11:34 p.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41769/ > --- > > (Updated Dec. 31, 2015, 11:34 p.m.) > > > Review request for mesos, Alexander Rukletsov, Klaus Ma, and Neil Conway. > > > Repository: mesos > > > Description > --- > > Made allocator traverse all roles for quota allocation. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > 7f900c4e024485704d79e57ae22407557598fe6c > > Diff: https://reviews.apache.org/r/41769/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Guangya Liu > >
Re: Review Request 41949: Replaced `QuotaInfo` with `Quota` in allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41949/ --- (Updated Jan. 12, 2016, 9:33 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere. Changes --- Rebased. Bugs: MESOS-3979 https://issues.apache.org/jira/browse/MESOS-3979 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/master/allocator.hpp fcebcab71c50a725ca4e635c03c29eed2a406bc3 src/master/allocator/mesos/allocator.hpp 50ef3b20f34bc6d87cbeccabcebec9a5031a6554 src/master/allocator/mesos/hierarchical.hpp 86ea5a402ed67f8f22f11d5730147cd907d66a08 src/master/allocator/mesos/hierarchical.cpp df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f src/master/quota_handler.cpp 134a93b1d1b6e050aa8a5037ffbec2cc305b0694 src/tests/allocator.hpp 9bdfaecf1a148f113ad52956b50ed7cabe0902ef src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59 src/tests/master_quota_tests.cpp 776a168254af6fa8a5d87d4580b35d83f2d5909a Diff: https://reviews.apache.org/r/41949/diff/ Testing --- `make check` on Mac OS 10.10.5 Thanks, Alexander Rukletsov
Re: Review Request 41950: Cleaned up hierarchical allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41950/ --- (Updated Jan. 12, 2016, 9:34 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere. Changes --- Rebased; addressed issues. Repository: mesos Description --- Changes made: - empty resource map promoted to a const class field; - removed variable numeric suffixes where appropriate; - added const where appropriate. Diffs (updated) - src/tests/hierarchical_allocator_tests.cpp e044f832c2c16e53e663c6ced5452649bb0dcb59 Diff: https://reviews.apache.org/r/41950/diff/ Testing --- `make check` on Mac OS 10.10.5 Thanks, Alexander Rukletsov
Re: Review Request 41947: Changed signature of `QuotaInfo` validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41947/ --- (Updated Jan. 12, 2016, 9:32 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere. Changes --- Rebased. Bugs: MESOS-4058 https://issues.apache.org/jira/browse/MESOS-4058 Repository: mesos Description --- See summary. Diffs (updated) - src/master/quota.hpp 7a728e3d2e66c4d0fd3b5dbf0fdc854363bae53e src/master/quota.cpp df2cc5877160df101222756739f042895ceacc95 src/master/quota_handler.cpp 134a93b1d1b6e050aa8a5037ffbec2cc305b0694 Diff: https://reviews.apache.org/r/41947/diff/ Testing --- `make check` on Mac OS 10.10.5 Thanks, Alexander Rukletsov
Re: Review Request 41938: Cleaned up quota tests.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41938/
---
(Updated Jan. 12, 2016, 9:33 p.m.)
Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
Changes
---
Rebased; addressed Jörg's issues.
Repository: mesos
Description
---
Changes made:
- removed extra ';' in resources string;
- renamed variables for clarity;
- updated comments;
- removed explicit conversion to `Resources`;
- wrapped test cases in {} blocks.
Diffs (updated)
-
src/tests/master_quota_tests.cpp 776a168254af6fa8a5d87d4580b35d83f2d5909a
src/tests/registrar_tests.cpp 6064621a001d66423f9b2dc7b749b67d9fc4bc13
Diff: https://reviews.apache.org/r/41938/diff/
Testing
---
`make check` on Mac OS 10.10.5
Thanks,
Alexander Rukletsov
Re: Review Request 41948: Ensured `QuotaInfo` is valid in registrar tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41948/ --- (Updated Jan. 12, 2016, 9:33 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere. Changes --- Rebased. Bugs: MESOS-4058 https://issues.apache.org/jira/browse/MESOS-4058 Repository: mesos Description --- Resources in `QuotaInfo` protobuf must not specify role, hence remove all occurrences of `flatten()` and add explicit validation. Diffs (updated) - src/tests/registrar_tests.cpp 6064621a001d66423f9b2dc7b749b67d9fc4bc13 Diff: https://reviews.apache.org/r/41948/diff/ Testing --- `make check` on Mac OS 10.10.5 Thanks, Alexander Rukletsov
Re: Review Request 41648: Used initializer list c-tor for brevity.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41648/ --- (Updated Jan. 12, 2016, 9:33 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere. Changes --- Rebased. Repository: mesos Description --- Used initializer list c-tor for brevity. Diffs (updated) - src/master/allocator/mesos/hierarchical.cpp df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f Diff: https://reviews.apache.org/r/41648/diff/ Testing --- `make check` on Mac OS 10.10.4 Thanks, Alexander Rukletsov
Re: Review Request 41937: Factored out parsing `QuotaInfo` from JSON into a function.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41937/ --- (Updated Jan. 12, 2016, 9:32 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere. Changes --- Rebased. Bugs: MESOS-4058 https://issues.apache.org/jira/browse/MESOS-4058 Repository: mesos Description --- See summary. Diffs (updated) - src/master/quota_handler.cpp 134a93b1d1b6e050aa8a5037ffbec2cc305b0694 Diff: https://reviews.apache.org/r/41937/diff/ Testing --- `make check` on Mac OS 10.10.5 Thanks, Alexander Rukletsov
Re: Review Request 41936: Required role in set quota request explicitly.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41936/ --- (Updated Jan. 12, 2016, 9:32 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere. Changes --- Rebased. Bugs: MESOS-4058 https://issues.apache.org/jira/browse/MESOS-4058 Repository: mesos Description --- A set quota request must provide a role, which now must be passed as a top-level field in the request JSON and not in `Resource` objects. Diffs (updated) - src/master/quota_handler.cpp 134a93b1d1b6e050aa8a5037ffbec2cc305b0694 src/tests/master_quota_tests.cpp 776a168254af6fa8a5d87d4580b35d83f2d5909a src/tests/role_tests.cpp 373ae267b85588fe491ab0a0ce8aa195f971aac3 Diff: https://reviews.apache.org/r/41936/diff/ Testing --- `make check` on Mac OS 10.10.5 Thanks, Alexander Rukletsov
Review Request 42221: Removed references to wDRF from allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42221/ --- Review request for mesos, Ben Mahler and Joris Van Remoortere. Repository: mesos Description --- Since wDRF is encapsulated in the choice of sorter, it should not be mentioned in the allocator code. Diffs - src/master/allocator/mesos/hierarchical.cpp df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f Diff: https://reviews.apache.org/r/42221/diff/ Testing --- `make check` on Mac OS 10.10.4 Thanks, Alexander Rukletsov
Review Request 42222: Added a comment on allocator recovery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4/ --- Review request for mesos, Ben Mahler and Joris Van Remoortere. Repository: mesos Description --- See summary. Diffs - src/master/allocator/mesos/hierarchical.cpp df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f Diff: https://reviews.apache.org/r/4/diff/ Testing --- None: not a functional change. Thanks, Alexander Rukletsov
Re: Review Request 48038: Updated `LocalAuthorizer` to consolidate to the `UPDATE_QUOTA` action.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48038/#review135548 --- src/authorizer/local/authorizer.cpp (line 349) <https://reviews.apache.org/r/48038/#comment200573> Introducing `approved()` looks like a general enhancement. Maybe do it in a separate patch? src/authorizer/local/authorizer.cpp (lines 364 - 367) <https://reviews.apache.org/r/48038/#comment200574> You make these now private. Is it on purpose? Do you want to do it in a separate patch explaining the reasoning? - Alexander Rukletsov On May 30, 2016, 1:17 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48038/ > --- > > (Updated May 30, 2016, 1:17 p.m.) > > > Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Vinod > Kone, and Zhitao Li. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/authorizer/local/authorizer.cpp > 547bbdd6c3605eadd23d2d2717a3fd362a616de5 > > Diff: https://reviews.apache.org/r/48038/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 48039: Updated `QuotaHandler` to only one authorization request per action.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48039/#review135549 --- src/master/master.hpp <https://reviews.apache.org/r/48039/#comment200577> Though this is currently not necessary, could you please leave a todo here, that after the deprecation cycle we should introduce this method instead of other two below? src/master/quota_handler.cpp (line 40) <https://reviews.apache.org/r/48039/#comment200575> Let's add a todo here that this dependency is temporary. src/master/quota_handler.cpp (line 500) <https://reviews.apache.org/r/48039/#comment200576> Please wrap for consistency. src/master/quota_handler.cpp (line 533) <https://reviews.apache.org/r/48039/#comment200578> ditto - Alexander Rukletsov On May 30, 2016, 1:17 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48039/ > --- > > (Updated May 30, 2016, 1:17 p.m.) > > > Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Vinod > Kone, and Zhitao Li. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/master.hpp eeeccdfdfd296c2a484764e887564f2e065cfd14 > src/master/quota_handler.cpp 04639efac8a8b30dbc7cbb2ce5c17d2a88f924b3 > > Diff: https://reviews.apache.org/r/48039/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 48037: Removed `SET_QUOTA_WITH_ROLE` and `REMOVE_QUOTA_WITH_PRINCIPAL` actions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48037/#review135547 --- include/mesos/authorizer/authorizer.proto <https://reviews.apache.org/r/48037/#comment200572> Instead of removing, let's comment out these fields or use https://developers.google.com/protocol-buffers/docs/proto#reserved to mark removed fields and their tags. However, since this enum is introduced in 0.29, we can simply update the tags, right? - Alexander Rukletsov On May 30, 2016, 1:17 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48037/ > --- > > (Updated May 30, 2016, 1:17 p.m.) > > > Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Vinod > Kone, and Zhitao Li. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > include/mesos/authorizer/authorizer.hpp > ed5f9e73661e25a83722cf1e408ae61023cd4a21 > include/mesos/authorizer/authorizer.proto > 4478bbd3c8f5c1fb862c2c6bd450689d870f7059 > > Diff: https://reviews.apache.org/r/48037/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 46093: Enhanced the error message for invalid duration unit.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46093/#review135551 --- Fix it, then Ship it! I'll fix the issue and commit it for you. 3rdparty/stout/include/stout/duration.hpp (lines 70 - 72) <https://reviews.apache.org/r/46093/#comment200580> How about ``` return Error( "Unknown duration unit '" + unit + "'; supported units are" " 'ns', 'us', 'ms', 'secs', 'mins', 'hrs', 'days', and 'weeks'"); ``` - Alexander Rukletsov On May 23, 2016, 3:41 a.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46093/ > --- > > (Updated May 23, 2016, 3:41 a.m.) > > > Review request for mesos, Alexander Rukletsov and Vinod Kone. > > > Bugs: MESOS-5179 > https://issues.apache.org/jira/browse/MESOS-5179 > > > Repository: mesos > > > Description > --- > > Enhanced the error message for invalid duration unit. > > > Diffs > - > > 3rdparty/stout/include/stout/duration.hpp > aea280830731a0e8c1aacd3fb6aef27291cc6f0b > > Diff: https://reviews.apache.org/r/46093/diff/ > > > Testing > --- > > LiuGuangyas-MacBook-Pro:build gyliu$ ./bin/mesos-master.sh --offer_timeout=4cc > Failed to load flag 'offer_timeout': Failed to load value '4cc': Unknown > duration unit 'cc', the supported units are 'ns', 'us', 'ms', 'secs', 'mins', > 'hrs', 'days' and 'weeks' > > > Thanks, > > Guangya Liu > >
Re: Review Request 48039: Updated `QuotaHandler` to only one authorization request per action.
> On May 30, 2016, 1:22 p.m., Till Toenshoff wrote: > > src/master/quota_handler.cpp, lines 575-580 > > <https://reviews.apache.org/r/48039/diff/1/?file=1400970#file1400970line575> > > > > Is there a JIRA around introducing LocalAuthorizer specific deprecation > > cycles - I would like to understand why we are not pushing this request out > > to any authorizer (module). There is no JIRA afaik. In short, the reason why local authorizer is "special" is because the authorizer interface has been refactored in 0.29 and hence we can simply avoid introducing enum values corresponding to the quota authz actions being deprecated in 0.29. However, we still must support related ACLs, which are relevant only to local authorizer. In one sentence: any authorizer (module) is revamped in 0.29 hence no deprecation is needed, while LocalAuthorizer should go through deprecation. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48039/#review135510 --- On May 30, 2016, 1:17 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48039/ > --- > > (Updated May 30, 2016, 1:17 p.m.) > > > Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Vinod > Kone, and Zhitao Li. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/master.hpp eeeccdfdfd296c2a484764e887564f2e065cfd14 > src/master/quota_handler.cpp 04639efac8a8b30dbc7cbb2ce5c17d2a88f924b3 > > Diff: https://reviews.apache.org/r/48039/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 47876: Updated comments for authorization::Object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47876/#review135543 --- include/mesos/authorizer/authorizer.proto (lines 38 - 40) <https://reviews.apache.org/r/47876/#comment200569> I think we should aim for a more general comment here. Since we are not employing the enum trick, it is possible to set multiple fields in this proto. I would like this comment to steer away from highliting one particular field (`value`). - Alexander Rukletsov On May 26, 2016, 8:25 a.m., Adam B wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47876/ > --- > > (Updated May 26, 2016, 8:25 a.m.) > > > Review request for mesos, Joerg Schad and Michael Park. > > > Repository: mesos > > > Description > --- > > Updated comments for authorization::Object. > > > Diffs > - > > include/mesos/authorizer/authorizer.proto > 02d1a01d57cf34b38524f4368187878b03343537 > > Diff: https://reviews.apache.org/r/47876/diff/ > > > Testing > --- > > > Thanks, > > Adam B > >
Review Request 49089: Removed a superfluous future repair invocation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49089/ --- Review request for mesos, Joerg Schad and Till Toenshoff. Repository: mesos Description --- There is no much value in a repair call here if it neither uses the original failure, nor tries to actually repair the future. In this case, repair always overwrites the failure returned from the lambda. Diffs - src/slave/slave.cpp b41f46b6c7bdbf58549635a6f345b4b5f98d5c52 Diff: https://reviews.apache.org/r/49089/diff/ Testing --- make check Thanks, Alexander Rukletsov
Re: Review Request 48947: Updated documentation table in authorization.md to follow styleguide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48947/#review138813 --- Fix it, then Ship it! Thanks for following up! docs/authorization.md (line 200) <https://reviews.apache.org/r/48947/#comment204003> Let's remove "for roles". docs/authorization.md (lines 207 - 208) <https://reviews.apache.org/r/48947/#comment204004> Let's remove "for roles". docs/authorization.md (line 218) <https://reviews.apache.org/r/48947/#comment204005> Ditto. - Alexander Rukletsov On June 20, 2016, 9:37 a.m., Alexander Rojas wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48947/ > --- > > (Updated June 20, 2016, 9:37 a.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-4785 > https://issues.apache.org/jira/browse/MESOS-4785 > > > Repository: mesos > > > Description > --- > > Updates the table of authorization actions in `authorization.doc` from > a MarkDown table to an HTML one as specified in the MarkDown guidelines. > > > Diffs > - > > docs/authorization.md 58f8a2a935c45b7a9b2e362d733ebb2abc0925c8 > > Diff: https://reviews.apache.org/r/48947/diff/ > > > Testing > --- > > Rendered in github.com > > > Thanks, > > Alexander Rojas > >
Review Request 49062: Fixed lambda capture list for consistency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49062/ --- Review request for mesos, Joerg Schad and Michael Park. Repository: mesos Description --- Our jsonify library consumes the lambdas immediately, without saving them internally or running asynchronously. Hence it is OK to capture by reference in this case. Diffs - src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e Diff: https://reviews.apache.org/r/49062/diff/ Testing --- make check on Mac OS 10.10.4 Thanks, Alexander Rukletsov
Review Request 49060: Removed unused approver from tasks authorization continuation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49060/ --- Review request for mesos, Joerg Schad and Michael Park. Repository: mesos Description --- See summary. Diffs - src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e Diff: https://reviews.apache.org/r/49060/diff/ Testing --- make check on Mac OS 10.10.4 Thanks, Alexander Rukletsov
Review Request 49061: Removed the unused copy of an `ObjectApprover` in a lambda.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49061/ --- Review request for mesos, Joerg Schad and Michael Park. Repository: mesos Description --- See summary. Diffs - src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e Diff: https://reviews.apache.org/r/49061/diff/ Testing --- make check on Mac OS 10.10.4 Thanks, Alexander Rukletsov
Review Request 49063: Updating formatting of some lambda calls for readability.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49063/ --- Review request for mesos, Joerg Schad and Michael Park. Repository: mesos Description --- See summary. Diffs - src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e Diff: https://reviews.apache.org/r/49063/diff/ Testing --- make check on Mac OS 10.10.4 Thanks, Alexander Rukletsov
Review Request 49059: Fixed the test suite build for Apple clang-600.0.54.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49059/
---
Review request for mesos, Jie Yu, Michael Park, and Till Toenshoff.
Repository: mesos
Description
---
Apparently, Apple clang-600.0.54, which is the stock compiler in
10.10.4, does not support passing `{}` as an empty set into a
function.
Diffs
-
src/tests/log_tests.cpp 362d9dd06b5fb54c3c4ed20426b03676de4ccc69
Diff: https://reviews.apache.org/r/49059/diff/
Testing
---
make check on Mac OS 10.10.4
Thanks,
Alexander Rukletsov
Re: Review Request 49063: Updating formatting of some lambda calls for readability.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49063/ --- (Updated June 22, 2016, 2:23 p.m.) Review request for mesos, Joerg Schad and Michael Park. Repository: mesos Description --- See summary. Diffs (updated) - src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e Diff: https://reviews.apache.org/r/49063/diff/ Testing --- make check on Mac OS 10.10.4 Thanks, Alexander Rukletsov
Review Request 49088: Replaced default capture by value by explicit capture by value.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49088/ --- Review request for mesos, Jay Guo, Joris Van Remoortere, and Michael Park. Repository: mesos Description --- When default capture by value is used in a lambda, it's hard to reason within what context the lambda has to be executed without auditing the code. Diffs - src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e Diff: https://reviews.apache.org/r/49088/diff/ Testing --- make check Thanks, Alexander Rukletsov
Re: Review Request 48733: Updated TODO and comments refering to 0.29.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48733/#review137793 --- Fix it, then Ship it! src/authorizer/local/authorizer.cpp (line 352) <https://reviews.apache.org/r/48733/#comment202979> According to the changelog, we call it 1.0.0 officially. Could you please update? - Alexander Rukletsov On June 15, 2016, 3:21 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48733/ > --- > > (Updated June 15, 2016, 3:21 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Repository: mesos > > > Description > --- > > After the decision to rename 0.29 to 1.0 we should > update comments and TODOs refering to 0.29. > Note we skipped comments which used 0.29 as a > target version. > > > Diffs > - > > src/authorizer/local/authorizer.cpp > 2186f0e027946862783e739ece4acbc17dff7d6f > src/credentials/credentials.hpp 35918ef9a9b66d5ccbc3c7a490762319e6fc5445 > src/docker/executor.hpp d20a8fbbce57d4458e2162381e783f53d2c413fb > src/docker/executor.cpp 29bdb1b5a9078325b016cc5da8ab40002166e13d > src/launcher/executor.cpp 346aa30c50a21636ced5dfb33942d66feb4eb9c8 > src/master/allocator/mesos/metrics.hpp > 06fdd1633f63c005230e1cc5fafdad358a0ac254 > src/master/quota_handler.cpp 06cf0d3f24844ec9e884237db3f33145ff406e80 > src/module/manager.cpp 8b408bf1e3107c8c60953b9726fe78b903ec447e > src/slave/containerizer/docker.cpp bd4f5930684208d532f4ad1721aae353d309c0a7 > src/slave/flags.hpp 17acf56bc1905cb1173c575accfb9bda8abd > src/slave/flags.cpp 7b0e347772646c177c1e18334633a71bc6cedb85 > src/tests/authorization_tests.cpp 217f36f38b589f6ee9df36c1c5d9b4fbb5b53033 > src/tests/master_quota_tests.cpp 62aaafe7b11a7df1ca7bc1f2f29af4cb626fc35d > > Diff: https://reviews.apache.org/r/48733/diff/ > > > Testing > --- > > > Thanks, > > Joerg Schad > >
Re: Review Request 48733: Updated TODO and comments refering to 0.29.
> On June 15, 2016, 6:32 p.m., Alexander Rukletsov wrote: > > src/authorizer/local/authorizer.cpp, line 352 > > <https://reviews.apache.org/r/48733/diff/1/?file=1420032#file1420032line352> > > > > According to the changelog, we call it 1.0.0 officially. Could you > > please update? After second though, patch version number is irrelevant for deprecation. Dropping the comment. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48733/#review137793 --- On June 15, 2016, 3:21 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48733/ > --- > > (Updated June 15, 2016, 3:21 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Repository: mesos > > > Description > --- > > After the decision to rename 0.29 to 1.0 we should > update comments and TODOs refering to 0.29. > Note we skipped comments which used 0.29 as a > target version. > > > Diffs > - > > src/authorizer/local/authorizer.cpp > 2186f0e027946862783e739ece4acbc17dff7d6f > src/credentials/credentials.hpp 35918ef9a9b66d5ccbc3c7a490762319e6fc5445 > src/docker/executor.hpp d20a8fbbce57d4458e2162381e783f53d2c413fb > src/docker/executor.cpp 29bdb1b5a9078325b016cc5da8ab40002166e13d > src/launcher/executor.cpp 346aa30c50a21636ced5dfb33942d66feb4eb9c8 > src/master/allocator/mesos/metrics.hpp > 06fdd1633f63c005230e1cc5fafdad358a0ac254 > src/master/quota_handler.cpp 06cf0d3f24844ec9e884237db3f33145ff406e80 > src/module/manager.cpp 8b408bf1e3107c8c60953b9726fe78b903ec447e > src/slave/containerizer/docker.cpp bd4f5930684208d532f4ad1721aae353d309c0a7 > src/slave/flags.hpp 17acf56bc1905cb1173c575accfb9bda8abd > src/slave/flags.cpp 7b0e347772646c177c1e18334633a71bc6cedb85 > src/tests/authorization_tests.cpp 217f36f38b589f6ee9df36c1c5d9b4fbb5b53033 > src/tests/master_quota_tests.cpp 62aaafe7b11a7df1ca7bc1f2f29af4cb626fc35d > > Diff: https://reviews.apache.org/r/48733/diff/ > > > Testing > --- > > > Thanks, > > Joerg Schad > >
Re: Review Request 48733: Updated TODO and comments refering to 0.29.
> On June 15, 2016, 6:55 p.m., Alexander Rukletsov wrote: > > src/module/manager.cpp, line 100 > > <https://reviews.apache.org/r/48733/diff/1/?file=1420039#file1420039line100> > > > > Did you leave out that 0.29.0 on purpose? Fixed in an addendum commit: a1eb8b8df037aadbfa9cf60c70f8e8fda209138b - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48733/#review137802 --- On June 15, 2016, 6:59 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48733/ > --- > > (Updated June 15, 2016, 6:59 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Repository: mesos > > > Description > --- > > After the decision to rename 0.29 to 1.0 we should > update comments and TODOs refering to 0.29. > Note we skipped comments which used 0.29 as a > target version. > > > Diffs > - > > src/authorizer/local/authorizer.cpp > 2186f0e027946862783e739ece4acbc17dff7d6f > src/credentials/credentials.hpp 35918ef9a9b66d5ccbc3c7a490762319e6fc5445 > src/docker/executor.hpp d20a8fbbce57d4458e2162381e783f53d2c413fb > src/docker/executor.cpp 29bdb1b5a9078325b016cc5da8ab40002166e13d > src/launcher/executor.cpp 346aa30c50a21636ced5dfb33942d66feb4eb9c8 > src/master/allocator/mesos/metrics.hpp > 06fdd1633f63c005230e1cc5fafdad358a0ac254 > src/master/quota_handler.cpp 06cf0d3f24844ec9e884237db3f33145ff406e80 > src/module/manager.cpp 8b408bf1e3107c8c60953b9726fe78b903ec447e > src/slave/containerizer/docker.cpp bd4f5930684208d532f4ad1721aae353d309c0a7 > src/slave/flags.hpp 17acf56bc1905cb1173c575accfb9bda8abd > src/slave/flags.cpp 7b0e347772646c177c1e18334633a71bc6cedb85 > src/tests/authorization_tests.cpp 217f36f38b589f6ee9df36c1c5d9b4fbb5b53033 > src/tests/master_quota_tests.cpp 62aaafe7b11a7df1ca7bc1f2f29af4cb626fc35d > > Diff: https://reviews.apache.org/r/48733/diff/ > > > Testing > --- > > None: Not a functional change. > > > Thanks, > > Joerg Schad > >
Re: Review Request 48733: Updated TODO and comments refering to 0.29.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48733/#review137802 --- Fix it, then Ship it! I'll fix the last outstanding issue and commit this shortly. src/module/manager.cpp (line 100) <https://reviews.apache.org/r/48733/#comment202988> Did you leave out that 0.29.0 on purpose? - Alexander Rukletsov On June 15, 2016, 3:21 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48733/ > --- > > (Updated June 15, 2016, 3:21 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Repository: mesos > > > Description > --- > > After the decision to rename 0.29 to 1.0 we should > update comments and TODOs refering to 0.29. > Note we skipped comments which used 0.29 as a > target version. > > > Diffs > - > > src/authorizer/local/authorizer.cpp > 2186f0e027946862783e739ece4acbc17dff7d6f > src/credentials/credentials.hpp 35918ef9a9b66d5ccbc3c7a490762319e6fc5445 > src/docker/executor.hpp d20a8fbbce57d4458e2162381e783f53d2c413fb > src/docker/executor.cpp 29bdb1b5a9078325b016cc5da8ab40002166e13d > src/launcher/executor.cpp 346aa30c50a21636ced5dfb33942d66feb4eb9c8 > src/master/allocator/mesos/metrics.hpp > 06fdd1633f63c005230e1cc5fafdad358a0ac254 > src/master/quota_handler.cpp 06cf0d3f24844ec9e884237db3f33145ff406e80 > src/module/manager.cpp 8b408bf1e3107c8c60953b9726fe78b903ec447e > src/slave/containerizer/docker.cpp bd4f5930684208d532f4ad1721aae353d309c0a7 > src/slave/flags.hpp 17acf56bc1905cb1173c575accfb9bda8abd > src/slave/flags.cpp 7b0e347772646c177c1e18334633a71bc6cedb85 > src/tests/authorization_tests.cpp 217f36f38b589f6ee9df36c1c5d9b4fbb5b53033 > src/tests/master_quota_tests.cpp 62aaafe7b11a7df1ca7bc1f2f29af4cb626fc35d > > Diff: https://reviews.apache.org/r/48733/diff/ > > > Testing > --- > > > Thanks, > > Joerg Schad > >
Re: Review Request 48787: Fixed chronos link in documentation.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48787/#review137967
---
Ship it!
Ship It!
- Alexander Rukletsov
On June 16, 2016, 12:19 p.m., Joerg Schad wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48787/
> ---
>
> (Updated June 16, 2016, 12:19 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> The previous link ('airbnb/chronos') is just a mirrow of 'mesos/airbnb').
>
>
> Diffs
> -
>
> docs/frameworks.md 632521893abdd2a0100c1acbc2219753811146ac
>
> Diff: https://reviews.apache.org/r/48787/diff/
>
>
> Testing
> ---
>
> checked link.
>
>
> Thanks,
>
> Joerg Schad
>
>
Re: Review Request 46872: Updated quota.md and weights.md for set quota and update weight.
On May 2, 2016, 4:01 p.m., Guangya Liu wrote:
> > Could you check whether this is an issue also with other endpoints?
>
> Guangya Liu wrote:
> Thanks Joerg, others are using `JSON String` in the `curl` command and
> only quota and weight need some update here.
If others using JSON in examples, why should we update weights and quota to use
files?
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46872/#review131319
---
On May 2, 2016, 10:35 p.m., Guangya Liu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46872/
> ---
>
> (Updated May 2, 2016, 10:35 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, and haosdent huang.
>
>
> Bugs: MESOS-5313
> https://issues.apache.org/jira/browse/MESOS-5313
>
>
> Repository: mesos
>
>
> Description
> ---
>
> There should be a @ before the json file when using "curl -d".
>
>
> Diffs
> -
>
> docs/quota.md 797e134605381ae576d9aa93875e0314889ab047
> docs/weights.md 59d1579ed691524185c52ccd0bc26eadfbe167c2
>
> Diff: https://reviews.apache.org/r/46872/diff/
>
>
> Testing
> ---
>
> root@mesos002:~/test# cat quota.json
> {
> "role": "role1",
> "guarantee": [{
> "name": "cpus",
> "type": "SCALAR",
> "scalar": {
> "value": 1
> }
> }, {
> "name": "mem",
> "type": "SCALAR",
> "scalar": {
> "value": 128
> }
> }]
> }
>
> root@mesos002:~/test# curl -d @quota.json -X POST
> http://192.168.56.12:5050/quota
>
> root@mesos002:~/test# curl -X GET http://192.168.56.12:5050/quota
> 2>/dev/null|python -m json.tool
> {
> "infos": [
> {
> "guarantee": [
> {
> "name": "cpus",
> "role": "*",
> "scalar": {
> "value": 1.0
> },
> "type": "SCALAR"
> },
> {
> "name": "mem",
> "role": "*",
> "scalar": {
> "value": 128.0
> },
> "type": "SCALAR"
> }
> ],
> "role": "role1"
> }
> ]
> }
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 46872: Updated quota.md and weights.md for set quota and update weight.
> On May 5, 2016, 6:14 a.m., Zhiwei Chen wrote:
> > Here I think it's ok in document, since -d supports raw data and a file
> > path. But this patch makes the doc more friendly to beginners.
Indeed, both are fine. The only thing that concerns me is consistency across
our examples; consistency is an important part of making our docs friendly to
beginners : ).
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46872/#review131789
---
On May 2, 2016, 10:35 p.m., Guangya Liu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46872/
> ---
>
> (Updated May 2, 2016, 10:35 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, and haosdent huang.
>
>
> Bugs: MESOS-5313
> https://issues.apache.org/jira/browse/MESOS-5313
>
>
> Repository: mesos
>
>
> Description
> ---
>
> There should be a @ before the json file when using "curl -d".
>
>
> Diffs
> -
>
> docs/quota.md 797e134605381ae576d9aa93875e0314889ab047
> docs/weights.md 59d1579ed691524185c52ccd0bc26eadfbe167c2
>
> Diff: https://reviews.apache.org/r/46872/diff/
>
>
> Testing
> ---
>
> root@mesos002:~/test# cat quota.json
> {
> "role": "role1",
> "guarantee": [{
> "name": "cpus",
> "type": "SCALAR",
> "scalar": {
> "value": 1
> }
> }, {
> "name": "mem",
> "type": "SCALAR",
> "scalar": {
> "value": 128
> }
> }]
> }
>
> root@mesos002:~/test# curl -d @quota.json -X POST
> http://192.168.56.12:5050/quota
>
> root@mesos002:~/test# curl -X GET http://192.168.56.12:5050/quota
> 2>/dev/null|python -m json.tool
> {
> "infos": [
> {
> "guarantee": [
> {
> "name": "cpus",
> "role": "*",
> "scalar": {
> "value": 1.0
> },
> "type": "SCALAR"
> },
> {
> "name": "mem",
> "role": "*",
> "scalar": {
> "value": 128.0
> },
> "type": "SCALAR"
> }
> ],
> "role": "role1"
> }
> ]
> }
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 48038: Updated `LocalAuthorizer` to consolidate to the `UPDATE_QUOTA` action.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48038/#review138135 --- Ship it! Ship It! - Alexander Rukletsov On June 16, 2016, 11:03 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48038/ > --- > > (Updated June 16, 2016, 11:03 p.m.) > > > Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Vinod > Kone, and Zhitao Li. > > > Bugs: MESOS-5628 > https://issues.apache.org/jira/browse/MESOS-5628 > > > Repository: mesos > > > Description > --- > > As described in https://reviews.apache.org/r/48039/, `QuotaHandler` is > updated to send only one authorization request per action. > > In this patch, we update the `LocalAuthorizer` to handle `UPDATE_QUOTA`. > Note that `QuotaHandler` encodes the type of the request (set or remove) > by setting the `value` field of `Object` to `"SetQuota"` or > `"RemoveQuota"`. The `LocalAuthorizer` uses this information to support > `ACL::SetQuota` and `ACL::RemoveQuota` throughout the deprecation cycle. > > > Diffs > - > > src/authorizer/local/authorizer.cpp > 558f332b3a5382cee64fb1dcf57b110e481b1efc > > Diff: https://reviews.apache.org/r/48038/diff/ > > > Testing > --- > > `make check` > > > Thanks, > > Michael Park > >
Re: Review Request 48040: Updated quota authorization tests to satisfy the new requirements.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48040/#review138137 --- Ship it! Ship It! - Alexander Rukletsov On June 16, 2016, 10:47 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48040/ > --- > > (Updated June 16, 2016, 10:47 p.m.) > > > Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Vinod > Kone, and Zhitao Li. > > > Bugs: MESOS-5628 > https://issues.apache.org/jira/browse/MESOS-5628 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/tests/authorization_tests.cpp c133f522743bc255c5d891dd8f5f04d5f209bba2 > > Diff: https://reviews.apache.org/r/48040/diff/ > > > Testing > --- > > `make check` > > > Thanks, > > Michael Park > >
Re: Review Request 48039: Updated `QuotaHandler` to send one authorization request per action.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48039/#review138131 --- Ship it! Ship It! - Alexander Rukletsov On June 16, 2016, 10:48 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48039/ > --- > > (Updated June 16, 2016, 10:48 p.m.) > > > Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Vinod > Kone, and Zhitao Li. > > > Bugs: MESOS-5628 > https://issues.apache.org/jira/browse/MESOS-5628 > > > Repository: mesos > > > Description > --- > > As described in MESOS-5628, this patch makes `QuotaHandler` only > send one authorization request per action. > > During the deprecation cycle of `ACL::SetQuota` and `ACL::RemoveQuota`, > the `LocalAuthorizer` for example still needs to know whether the action > is a set or remove. We remedy this situation by encoding type (set or > remove) by setting the `value` field in the `Object` with the string > `"SetQuota"` or `"RemoveQuota"`. With this approach, we can simply stop > setting the `value` field at the end of the deprecation cycle as opposed > to changing the public API. > > > Diffs > - > > src/master/master.hpp 72c60ef74ce57119a97cf8305182340a13c58c42 > src/master/quota_handler.cpp 7eeb60ed08deb2d7139423716e70e9acc92a0416 > > Diff: https://reviews.apache.org/r/48039/diff/ > > > Testing > --- > > `make check` > > > Thanks, > > Michael Park > >
Re: Review Request 48986: Updated a typo for GPU_RESOURCES.
> On June 21, 2016, 1:19 p.m., Alexander Rukletsov wrote: > > Why do you think it is a typo? > > Guangya Liu wrote: > I think that here we should "preventing non-GPU workloads from using > precious GPU resources", comments? But this is what the original comment says, no? If you think it is confusing, we can re-phrase it altogether. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48986/#review138826 --- On June 21, 2016, 12:47 p.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48986/ > --- > > (Updated June 21, 2016, 12:47 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-5634 > https://issues.apache.org/jira/browse/MESOS-5634 > > > Repository: mesos > > > Description > --- > > Updated a typo for GPU_RESOURCES. > > > Diffs > - > > include/mesos/mesos.proto e4bb313f9229f2101aa69acf82e367c01aeefb63 > include/mesos/v1/mesos.proto 8bba833c65c7f7b18fbdfe981280ef5d39f8c70b > > Diff: https://reviews.apache.org/r/48986/diff/ > > > Testing > --- > > > Thanks, > > Guangya Liu > >
Re: Review Request 48986: Updated a typo for GPU_RESOURCES.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48986/#review138826 --- Why do you think it is a typo? - Alexander Rukletsov On June 21, 2016, 12:47 p.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48986/ > --- > > (Updated June 21, 2016, 12:47 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-5634 > https://issues.apache.org/jira/browse/MESOS-5634 > > > Repository: mesos > > > Description > --- > > Updated a typo for GPU_RESOURCES. > > > Diffs > - > > include/mesos/mesos.proto e4bb313f9229f2101aa69acf82e367c01aeefb63 > include/mesos/v1/mesos.proto 8bba833c65c7f7b18fbdfe981280ef5d39f8c70b > > Diff: https://reviews.apache.org/r/48986/diff/ > > > Testing > --- > > > Thanks, > > Guangya Liu > >
Review Request 49049: Restored the continuation logic fix.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49049/ --- Review request for mesos, Jay Guo, Joerg Schad, and Vinod Kone. Repository: mesos Description --- Commit aa1cfd38f544ea9fa615200ebf7612e05ac32031 fixed a race in a continuation in endpoint authorisation, while commit 608e3823dbc19fabf70687840f27b1a628ac0cd6 unintentionally rolled it back. This patch restores the original fix. Diffs - src/master/http.cpp 3a53ef74f3e80e832804a21cebe0fc9e405420cb Diff: https://reviews.apache.org/r/49049/diff/ Testing --- make check Thanks, Alexander Rukletsov
Re: Review Request 48920: Updated the HTTP result returned by failures of authn/authz.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48920/#review138491
---
3rdparty/libprocess/src/process.cpp (line 3327)
<https://reviews.apache.org/r/48920/#comment203681>
As in r/48919/, let's not create an empty body.
3rdparty/libprocess/src/process.cpp (line 3387)
<https://reviews.apache.org/r/48920/#comment203682>
Ditto.
- Alexander Rukletsov
On June 19, 2016, 9:01 p.m., Till Toenshoff wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48920/
> ---
>
> (Updated June 19, 2016, 9:01 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar,
> Alexander Rojas, Benjamin Mahler, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-5637
> https://issues.apache.org/jira/browse/MESOS-5637
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Changes authentication and authorization happening on the libprocess
> level to be in line with failures possibly returned by Mesos
> authorization as currently implemented by the HTTPProxy::process
> function. The HTTP result returned on failures has changed from
> InternalServerError (500) towards ServiceNotAvailable (503) and now
> contains a message describing the problem.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/src/process.cpp
> 703f673a98102958c5e2b0c1833efad2ddc53ef8
>
> Diff: https://reviews.apache.org/r/48920/diff/
>
>
> Testing
> ---
>
> make check & functional testing.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 48919: Updated HTTPProxy to return a failure message in the HTTP result.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48919/#review138486
---
3rdparty/libprocess/src/process.cpp (lines 1217 - 1218)
<https://reviews.apache.org/r/48919/#comment203668>
In r/48918 you removed logging of the failed future, so now those failures
are not captured in the log. I would strongly suggest to restore logging here.
My preference would be `VLOG(3)`.
3rdparty/libprocess/src/process.cpp (line 1218)
<https://reviews.apache.org/r/48919/#comment203671>
In case future is not failed, you create a response with an empty body, but
with some headers ("Content-Length", "Content-Type") set. I believe this is not
what you want. How about:
```
Response response = future.isFailed() ?
ServiceUnavailable(future.failure() :
ServiceUnavailable();
socket_manager->send(response, request, socket);
```
3rdparty/libprocess/src/process.cpp (lines 1220 - 1221)
<https://reviews.apache.org/r/48919/#comment203672>
We tend to insert a blank line in this case : ).
- Alexander Rukletsov
On June 19, 2016, 9:02 p.m., Till Toenshoff wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48919/
> ---
>
> (Updated June 19, 2016, 9:02 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar,
> Alexander Rojas, Benjamin Mahler, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-5637
> https://issues.apache.org/jira/browse/MESOS-5637
>
>
> Repository: mesos
>
>
> Description
> ---
>
> see summary.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/src/process.cpp
> 703f673a98102958c5e2b0c1833efad2ddc53ef8
>
> Diff: https://reviews.apache.org/r/48919/diff/
>
>
> Testing
> ---
>
> make check & functional testing.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 48932: Updated CHANGELOG to include note about '503' on authorizer failures.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48932/#review138493 --- CHANGELOG (lines 160 - 161) <https://reviews.apache.org/r/48932/#comment203686> I would like us to mention two more things: the purpose of the change and what is being substituted. How about the following: [MESOS-5637] - Authorized endpoints consistently return `503 (Service Unavailable)` instead of `500 (Internal Server Error)` when the authenticator or the authorizer fails to process the request. - Alexander Rukletsov On June 19, 2016, 9 p.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48932/ > --- > > (Updated June 19, 2016, 9 p.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, > Alexander Rojas, Benjamin Mahler, Greg Mann, and Kapil Arya. > > > Bugs: MESOS-5637 > https://issues.apache.org/jira/browse/MESOS-5637 > > > Repository: mesos > > > Description > --- > > see summary. > > > Diffs > - > > CHANGELOG aed56300f1090c816ab2893c20e87fd37230a9a7 > > Diff: https://reviews.apache.org/r/48932/diff/ > > > Testing > --- > > > Thanks, > > Till Toenshoff > >
Re: Review Request 48919: Updated HTTPProxy to return a failure message in the HTTP result.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48919/#review138536 --- Fix it, then Ship it! 3rdparty/libprocess/src/process.cpp (line 1225) <https://reviews.apache.org/r/48919/#comment203734> Why not `future.failure?`? - Alexander Rukletsov On June 20, 2016, 5:50 a.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48919/ > --- > > (Updated June 20, 2016, 5:50 a.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, > Alexander Rojas, Benjamin Mahler, Greg Mann, and Kapil Arya. > > > Bugs: MESOS-5637 > https://issues.apache.org/jira/browse/MESOS-5637 > > > Repository: mesos > > > Description > --- > > see summary. > > > Diffs > - > > 3rdparty/libprocess/src/process.cpp > 703f673a98102958c5e2b0c1833efad2ddc53ef8 > > Diff: https://reviews.apache.org/r/48919/diff/ > > > Testing > --- > > make check (OSX and some Linux distros) > > > Thanks, > > Till Toenshoff > >
Re: Review Request 48923: Updated test to expect ServiceUnavailable.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48923/#review138539 --- Ship it! Ship It! - Alexander Rukletsov On June 20, 2016, 5:51 a.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48923/ > --- > > (Updated June 20, 2016, 5:51 a.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, > Alexander Rojas, Benjamin Mahler, Greg Mann, and Kapil Arya. > > > Bugs: MESOS-5637 > https://issues.apache.org/jira/browse/MESOS-5637 > > > Repository: mesos > > > Description > --- > > StatisticsEndpointGetResourceUsageFailed was expecting an > InternalServerError but instead it will now receive a > ServiceUnavailable. > > > Diffs > - > > src/tests/slave_tests.cpp ad36b74c12e52435b356bced1d15bd33983949e0 > > Diff: https://reviews.apache.org/r/48923/diff/ > > > Testing > --- > > make check (OSX and some Linux distros) > > > Thanks, > > Till Toenshoff > >
Re: Review Request 48920: Updated the HTTP result returned by failures of authn/authz.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48920/#review138538 --- Ship it! Ship It! - Alexander Rukletsov On June 20, 2016, 5:49 a.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48920/ > --- > > (Updated June 20, 2016, 5:49 a.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, > Alexander Rojas, Benjamin Mahler, Greg Mann, and Kapil Arya. > > > Bugs: MESOS-5637 > https://issues.apache.org/jira/browse/MESOS-5637 > > > Repository: mesos > > > Description > --- > > Changes authentication and authorization happening on the libprocess > level to be in line with failures possibly returned by Mesos > authorization as currently implemented by the HTTPProxy::process > function. The HTTP result returned on failures has changed from > InternalServerError (500) towards ServiceNotAvailable (503) and now > contains a message describing the problem. > > > Diffs > - > > 3rdparty/libprocess/src/process.cpp > 703f673a98102958c5e2b0c1833efad2ddc53ef8 > > Diff: https://reviews.apache.org/r/48920/diff/ > > > Testing > --- > > make check (OSX and some Linux distros) & functional testing. > > > Thanks, > > Till Toenshoff > >
Re: Review Request 48918: Removed explicit authorization results in Mesos.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48918/#review138537 --- Ship it! Ship It! - Alexander Rukletsov On June 20, 2016, 5:50 a.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48918/ > --- > > (Updated June 20, 2016, 5:50 a.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, Alexander Rojas, > Benjamin Mahler, Greg Mann, and Kapil Arya. > > > Bugs: MESOS-5637 > https://issues.apache.org/jira/browse/MESOS-5637 > > > Repository: mesos > > > Description > --- > > Removing the explicit authorization results as formerly returned by > some authorizer invocations in Mesos, the returned future falls > through/back to the HTTP proxy handler of libprocess. This change > allows us to reduce the amount of failure result handlers. > > > Diffs > - > > src/master/http.cpp a6beb1721958a77886f0aa535346e2ff33bd5d04 > src/slave/http.cpp 77834111bf51b58ba5e12f262b725202f7d3a7bf > > Diff: https://reviews.apache.org/r/48918/diff/ > > > Testing > --- > > make check (OSX and some Linux distros) > > > Thanks, > > Till Toenshoff > >
Re: Review Request 48932: Updated CHANGELOG to include note about '503' on authorizer failures.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48932/#review138541 --- Fix it, then Ship it! CHANGELOG (lines 160 - 161) <https://reviews.apache.org/r/48932/#comment203736> Backticks : ). I would also put http status code name under backticks. - Alexander Rukletsov On June 20, 2016, 1:03 a.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48932/ > --- > > (Updated June 20, 2016, 1:03 a.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, > Alexander Rojas, Benjamin Mahler, Greg Mann, and Kapil Arya. > > > Bugs: MESOS-5637 > https://issues.apache.org/jira/browse/MESOS-5637 > > > Repository: mesos > > > Description > --- > > see summary. > > > Diffs > - > > CHANGELOG aed56300f1090c816ab2893c20e87fd37230a9a7 > > Diff: https://reviews.apache.org/r/48932/diff/ > > > Testing > --- > > > Thanks, > > Till Toenshoff > >
Re: Review Request 48902: Move v1/master/allocator.proto to its own package.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48902/#review138546 --- To avoid confusion, we should actually change the namespace allocator code lives is as well. I've once started that effort (https://reviews.apache.org/r/29930/) but decided to discard because allocator is the master-only component. - Alexander Rukletsov On June 18, 2016, 6:36 a.m., Zhitao Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48902/ > --- > > (Updated June 18, 2016, 6:36 a.m.) > > > Review request for mesos, Anand Mazumdar and Vinod Kone. > > > Bugs: MESOS-5642 > https://issues.apache.org/jira/browse/MESOS-5642 > > > Repository: mesos > > > Description > --- > > Move v1/master/allocator.proto to its own package. > > > Diffs > - > > include/mesos/v1/maintenance/maintenance.proto > 053d72d7d9fa9ec8f572136020546fa4f955ab09 > include/mesos/v1/master/allocator.proto > cf416d173bc487aecbbec75c9ee391a54bf5327b > src/CMakeLists.txt d66186217c1319d4497640614ed4beee28602c38 > src/Makefile.am a4931560f1a5b3fbe41ea181477341d3ac459b58 > > Diff: https://reviews.apache.org/r/48902/diff/ > > > Testing > --- > > run `make` on Mac. > > > Thanks, > > Zhitao Li > >
Re: Review Request 48936: Explained why fields in the acts.Entity must be required.
> On June 20, 2016, 12:19 p.m., Till Toenshoff wrote: > > include/mesos/authorizer/acls.proto, lines 34-35 > > <https://reviews.apache.org/r/48936/diff/1/?file=1423750#file1423750line34> > > > > The above note seems to be targetted towards the users/consumers of > > these messages whereas the new note is targeted towards Mesos developers, > > warning them to not break the pattern. > > > > I feel we should change the style here to make that more obvious. > > > > How about: > > > > ``` > > NOTE: When adding new actions to these proto definitions that entities > > in these messages are `required`. Absent ... > > ``` Good! I'll change to ``` // NOTE: When adding new actions to these proto definitions, // declare entities in these messages as `required`. Absent // `Entity` is ambiguous: it can be interpreted as `NONE` or `ANY`. ``` - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48936/#review138507 ------- On June 20, 2016, 12:12 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48936/ > --- > > (Updated June 20, 2016, 12:12 a.m.) > > > Review request for mesos, Alexander Rojas and Till Toenshoff. > > > Bugs: MESOS-5588 > https://issues.apache.org/jira/browse/MESOS-5588 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > include/mesos/authorizer/acls.proto > f8e866b876c49e62a07f29edc66fc205ff33d618 > > Diff: https://reviews.apache.org/r/48936/diff/ > > > Testing > --- > > None: Not a functional change. > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 48936: Explained why fields in the acts.Entity must be required.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48936/ --- (Updated June 20, 2016, 1:35 p.m.) Review request for mesos, Alexander Rojas and Till Toenshoff. Changes --- Addressed comments. Bugs: MESOS-5588 https://issues.apache.org/jira/browse/MESOS-5588 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/authorizer/acls.proto f8e866b876c49e62a07f29edc66fc205ff33d618 Diff: https://reviews.apache.org/r/48936/diff/ Testing --- None: Not a functional change. Thanks, Alexander Rukletsov
Re: Review Request 48937: Added a TODO about possible security issues due to misspelled ACLs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48937/ --- (Updated June 20, 2016, 1:34 p.m.) Review request for mesos, Joerg Schad and Till Toenshoff. Changes --- Rebased. Bugs: MESOS-5588 https://issues.apache.org/jira/browse/MESOS-5588 Repository: mesos Description --- See summary. Diffs (updated) - src/authorizer/local/authorizer.cpp 60dfe10dbefa0e82c8f3c1012ef632d786109d1b Diff: https://reviews.apache.org/r/48937/diff/ Testing --- None: Not a functional change. Thanks, Alexander Rukletsov
Re: Review Request 48902: Move v1/master/allocator.proto to its own package.
> On June 20, 2016, 8:06 a.m., Alexander Rukletsov wrote: > > To avoid confusion, we should actually change the namespace allocator code > > lives is as well. I've once started that effort > > (https://reviews.apache.org/r/29930/) but decided to discard because > > allocator is the master-only component. > > Zhitao Li wrote: > Alexander, so what's your recommendation here? Given that there is only > one public visible message `InverseOfferStatus` in this protobuf at the > moment, I'd like to have a quick fix to make sure its namespace does not > change after v1.0 and catch the release deadline. > > If "allocator is the master-only component" is considered a good reason > to keep these code in master namespace, then the question is how to handle > maintenance which could have circular dependency to master components: maybe > we should just move this message to `mesos` namespace as other high level > messages like `Offer` or `TaskStatus`? > > haosdent huang wrote: > >make sure its namespace does not change after v1.0 and catch the release > deadline. > > +1 for this. I think all we don't want to break the V1 protobuf files > after 1.0 release > > I am still curious why could not put allocator.proto and master.proto > under the same folder. > If it is forbidden and we want to keep allocator.proto under master > namespace, I think we could > move `message InverseOfferStatus` from allocator.proto to master.proto > and then remove allocator.proto. > > Zhitao Li wrote: > @haosdent, the goal is here is to both make the protobuf structures > consistent between packages, and avoid possible circular dependency for > various languages. > > For golang, including a protobuf also generates an import to the package > included protobuf file is in. Therefore, `master` package imports > `maintenance`, and `maintenance` imports `master` (though this > allocator.proto file) and causes a circular dependency. > > After some thoughts, the only `InverseOfferStatus` message in > allocator.proto definitely should not belong to `master` package, because the > latter is pretty much used soly for the new operator HTTP API. Given that > this message spans all "allocator", "Mesos master" and "maintenance" logic > concepts, it's common enough to be promoted to `include/mesos/v1/mesos.proto` > and be put next to `message InverseOffer`. And if we move this one, we can > simply drop this allocator.proto file because it'll be empty. (Alternatively, > if we don't think it's common enough, moving it to > `include/mesos/v1/maintenance/maintenance.proto` is acceptable to me). > > Note that I'm only talking about the messages namespaced in v1. I agree > we should make non versioned messages consistent here, but that could be done > in a separate patch. > > haosdent huang wrote: > Got it, +1 for move `InverseOfferStatus` to `package mesos` which follow > `InverseOffer`. > Hi, @alexr Do you think if this is OK? It is fine to do the v1 change. I would like to avoid inconsistency, when some allocator-related types live in "master" namespace or package, and some in "allocator". Before you move, could you ask Joris or Joseph why they had put `InverseOfferStatus` into a separate proto in the first place? - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48902/#review138546 --- On June 18, 2016, 6:36 a.m., Zhitao Li wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48902/ > --- > > (Updated June 18, 2016, 6:36 a.m.) > > > Review request for mesos, Anand Mazumdar and Vinod Kone. > > > Bugs: MESOS-5642 > https://issues.apache.org/jira/browse/MESOS-5642 > > > Repository: mesos > > > Description > --- > > Move v1/master/allocator.proto to its own package. > > > Diffs > - > > include/mesos/v1/maintenance/maintenance.proto > 053d72d7d9fa9ec8f572136020546fa4f955ab09 > include/mesos/v1/master/allocator.proto > cf416d173bc487aecbbec75c9ee391a54bf5327b > src/CMakeLists.txt d66186217c1319d4497640614ed4beee28602c38 > src/Makefile.am a4931560f1a5b3fbe41ea181477341d3ac459b58 > > Diff: https://reviews.apache.org/r/48902/diff/ > > > Testing > --- > > run `make` on Mac. > > > Thanks, > > Zhitao Li > >
Re: Review Request 48037: Removed `SET_QUOTA_WITH_ROLE` and `REMOVE_QUOTA_WITH_PRINCIPAL` actions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48037/#review138050 --- Ship it! Ship It! - Alexander Rukletsov On June 16, 2016, 6:28 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48037/ > --- > > (Updated June 16, 2016, 6:28 p.m.) > > > Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Vinod > Kone, and Zhitao Li. > > > Repository: mesos > > > Description > --- > > Renamed `UPDATE_QUOTA_WITH_ROLE` to `UPDATE_QUOTA`. > > > Diffs > - > > include/mesos/authorizer/authorizer.hpp > 02d0d043bd3bf3956709b7c90db8cc4eeb0525ad > include/mesos/authorizer/authorizer.proto > ffbe4d0f2900601427398e3b1fb8ee478343f034 > > Diff: https://reviews.apache.org/r/48037/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 46875: Some cleanup in weights_handler.cpp.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46875/#review138209
---
I beleive `request.body` is supposed to be the contents, and not the file,
right?
- Alexander Rukletsov
On May 1, 2016, 6:52 a.m., Guangya Liu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46875/
> ---
>
> (Updated May 1, 2016, 6:52 a.m.)
>
>
> Review request for mesos, Adam B and Alexander Rukletsov.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> There is no need to add () for request body as there is already
> '' for the message.
>
>
> Diffs
> -
>
> src/master/weights_handler.cpp 07cc9920ca61f3e6e9e5b8be37884ed6f0d4f975
>
> Diff: https://reviews.apache.org/r/46875/diff/
>
>
> Testing
> ---
>
> Before fix:
> root@mesos002:~/test# curl -d weight.json -X PUT
> http://192.168.56.12:5050/weights
> Failed to parse update weights request JSON ('weight.json'): syntax error at
> line 1 near: weight.js
>
> After fix:
> root@mesos002:~/test# curl -d weight.json -X PUT
> http://192.168.56.12:5050/weights
> Failed to parse update weights request JSON 'weight.json': syntax error at
> line 1 near: weight.json
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 42040: Added Quota Operator Documentation.
> On Jan. 9, 2016, 4:09 a.m., Guangya Liu wrote: > > docs/quota.md, lines 306-310 > > <https://reviews.apache.org/r/42040/diff/2/?file=1187349#file1187349line306> > > > > 1) what about adding "update Quota" here and remove it from L189 > > 2) no Quota Limit but only guarantee > > > > Another point is that shall we put quota design document link here to > > naviagte someone there if they want to get some design detail for Quota? > > Joerg Schad wrote: > 1) Added it here (brief) with a link to L189. > 2) As we do not mention Quota Limits above, I would also not mention it > here (see our current definition of quota). > > Guangya Liu wrote: > OK Let's not mention the design doc in user docs. The proper place for a design doc is apache confluence. Updating, cleaning up, and moving the design doc to Confluence is on my list. I would suggest we do not block this review because of that. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42040/#review113601 --- On Jan. 8, 2016, 10:26 a.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42040/ > --- > > (Updated Jan. 8, 2016, 10:26 a.m.) > > > Review request for mesos, Alexander Rukletsov, Bernd Mathiske, Joris Van > Remoortere, and Neil Conway. > > > Bugs: MESOS-3877 > https://issues.apache.org/jira/browse/MESOS-3877 > > > Repository: mesos > > > Description > --- > > Added Quota Operator Documentation. > > > Diffs > - > > docs/home.md 6f0f4b9cb9d0da1f9960ebe7f36ce186c1317535 > docs/quota.md PRE-CREATION > > Diff: https://reviews.apache.org/r/42040/diff/ > > > Testing > --- > > Rendered version: https://gist.github.com/joerg84/a2c32e25d91e33045b56 > > > Thanks, > > Joerg Schad > >
Review Request 42255: Updated user documentation around HTTP response codes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42255/ --- Review request for mesos, Alexander Rojas and Till Toenshoff. Bugs: MESOS-4305 https://issues.apache.org/jira/browse/MESOS-4305 Repository: mesos Description --- Updated user documentation around HTTP response codes. Diffs - docs/persistent-volume.md f969975f242f7fc7a8b7a89e3e211d2201c9114a docs/reservation.md a5dbc0abd5d9aef04064dc2a1dff7eb118147e43 Diff: https://reviews.apache.org/r/42255/diff/ Testing --- None: not a functional change. Thanks, Alexander Rukletsov
Re: Review Request 42027: Changed HTTP responses from Unauthorized (401) to Forbidden (403).
> On Jan. 13, 2016, 3:05 p.m., Alexander Rukletsov wrote: > > I'm reading section 1.2 of RFC2617 and see the following paragraph: > > > > > If the origin server does not wish to accept the credentials sent > > > with a request, it SHOULD return a 401 (Unauthorized) response. The > > > response MUST include a WWW-Authenticate header field containing at > > > least one (possibly new) challenge applicable to the requested > > > resource... > > > > I would like to confirm my understanding of the proposed change is correct. > > > > Authentication is general to HTTP, we leverage HTTP headers and adhere > > RFCs. Authorization is entirely internal to Mesos and is not regulated by > > any standards. Currently we use `Unauthorized` for both authn and authz > > failures. Section 1.2 of RFC2617 hints that using `Unauthorized` for > > authentication is correct. However, because Mesos authorization is actually > > an implementation detail, we should use a more general return code, e.g. > > `Forbidden`. So far so good? (I can't help but think about putting an > > eastern egg in Mesos and sporadically return `402 Payment Required` for, > > say, dynamic reservation requests : ) ). > > > > If I read sections 10.4.4 and 10.4.4 of RFC2616 correctly, `Forbidden` > > should include the reason for the refusal. Do you think it makes sense to > > include reasons everywhere? Maybe even remove parameterless c-tor for > > `Forbidden` (then comparing status will be tricky, but maybe we should make > > `status` static anyway)? > > Alexander Rojas wrote: > Completely agreed, It would be better if we removed the default > constructor there and force to add the reasons. I'll open a JIRA for that. Great, thanks a bunch! > On Jan. 13, 2016, 3:05 p.m., Alexander Rukletsov wrote: > > src/master/http.cpp, lines 356-360 > > <https://reviews.apache.org/r/42027/diff/4/?file=1192104#file1192104line356> > > > > Could you please help me understand why this shoud be `Forbidden`? If > > my understanding is correct, it should stay `Unauthorized` no HTTP auth > > headers are provided and it has nothing to do with the Mesos authz. > > > > Here is what I see in the RFC: > > " If the origin server does not wish to accept the credentials sent > > with a request, it SHOULD return a 401 (Unauthorized) response." > > Alexander Rojas wrote: > Because this is not doing HTTP Authentication, and it wasn't returning > the header `WWW-Authenticate`. So we decided `Forbidden` was a more aptly > return code (Remember, this is the old cram_md5 authenticator). Ok, makes sense. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42027/#review114201 --- On Jan. 11, 2016, 3:42 p.m., Alexander Rojas wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42027/ > --- > > (Updated Jan. 11, 2016, 3:42 p.m.) > > > Review request for mesos, Alexander Rukletsov, Greg Mann, Joerg Schad, Jan > Schlicht, and Till Toenshoff. > > > Bugs: MESOS-4305 > https://issues.apache.org/jira/browse/MESOS-4305 > > > Repository: mesos > > > Description > --- > > It is a common patter within Mesos to return an HTTP 401 (Unauthorized) > response whenever the request is invalid for whatever reason. However, > according to the [RFC-2617 Section > 1.2](https://tools.ietf.org/html/rfc2617#section-1.2): > > The 401 (Unauthorized) response message is used by an origin server to > > challenge the authorization of a user agent. This response MUST include a > > WWW-Authenticate header field containing at least one challenge applicable > > to the requested resource. > > Meaning that despite the confusing name, the status code _401 Unauthorized_ > should be used only for authentication purposes. On the other hand, the > [RFC-2616 Section > 10.4.4](http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4) > states: > > _(403 Forbidden is returned when)_ The server understood the request, but > > is refusing to fulfill it. Authorization will not help and the request > > SHOULD NOT be repeated. If the request method was not HEAD and the server > > wishes to make public why the request has not been fulfilled, it SHOULD > > d
Re: Review Request 42221: Removed references to wDRF from allocator.
> On Jan. 13, 2016, 1:52 a.m., Guangya Liu wrote: > > src/master/allocator/mesos/hierarchical.cpp, line 1185 > > <https://reviews.apache.org/r/42221/diff/1/?file=1194999#file1194999line1185> > > > > s/framwork/frameworks > > Alexander Rukletsov wrote: > Why do you want plural here? > > Guangya Liu wrote: > It is just typo, I think that we can fix it here as it does not deserve > to file another jira to fix it, comments? It may be a grammatical mistake, but definitely not a typo: I put singular here on purpose. Only a single framework can accept a resource at a time. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42221/#review114126 ------- On Jan. 12, 2016, 11:06 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42221/ > --- > > (Updated Jan. 12, 2016, 11:06 p.m.) > > > Review request for mesos, Ben Mahler and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > Since wDRF is encapsulated in the choice of sorter, it should not be > mentioned in the allocator code. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f > > Diff: https://reviews.apache.org/r/42221/diff/ > > > Testing > --- > > `make check` on Mac OS 10.10.4 > > > Thanks, > > Alexander Rukletsov > >
Review Request 42305: Updated comments around sorters in the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42305/ --- Review request for mesos, Ben Mahler and Joris Van Remoortere. Bugs: MESOS-4128 https://issues.apache.org/jira/browse/MESOS-4128 Repository: mesos Description --- See summary. Diffs - src/master/allocator/mesos/hierarchical.hpp 5f08b6e7d18766d7d0f8350a280ec577b4895fb9 Diff: https://reviews.apache.org/r/42305/diff/ Testing --- None: not a functional change. Thanks, Alexander Rukletsov
Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42241/#review114994 --- src/tests/hook_tests.cpp (lines 396 - 399) <https://reviews.apache.org/r/42241/#comment175852> Why do you need a loop here? Isn't it enought to advance once and then settle? - Alexander Rukletsov On Jan. 18, 2016, 2:28 a.m., Jian Qiu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42241/ > --- > > (Updated Jan. 18, 2016, 2:28 a.m.) > > > Review request for mesos, Alexander Rukletsov and Timothy Chen. > > > Bugs: MESOS-4174 > https://issues.apache.org/jira/browse/MESOS-4174 > > > Repository: mesos > > > Description > --- > > Speed up HookTest.VerifySlaveLaunchExecutorHook. > > > Diffs > - > > src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 > > Diff: https://reviews.apache.org/r/42241/diff/ > > > Testing > --- > > Before > HookTest.VerifySlaveLaunchExecutorHook (5061 ms) > > After > HookTest.VerifySlaveLaunchExecutorHook (132 ms) > > > Thanks, > > Jian Qiu > >
Re: Review Request 42255: Updated user documentation around HTTP response codes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42255/ --- (Updated Jan. 18, 2016, 11:46 a.m.) Review request for mesos, Alexander Rojas and Till Toenshoff. Changes --- Rebased. Bugs: MESOS-4305 https://issues.apache.org/jira/browse/MESOS-4305 Repository: mesos Description --- Updated user documentation around HTTP response codes. Diffs (updated) - docs/persistent-volume.md f969975f242f7fc7a8b7a89e3e211d2201c9114a docs/reservation.md 57cd92ce5d8d25b3393763614e40155e17e8c06e Diff: https://reviews.apache.org/r/42255/diff/ Testing --- None: not a functional change. Thanks, Alexander Rukletsov
Re: Review Request 42221: Removed references to wDRF from allocator.
> On Jan. 16, 2016, 10:36 p.m., Joris Van Remoortere wrote: > > src/master/allocator/mesos/hierarchical.cpp, line 1245 > > <https://reviews.apache.org/r/42221/diff/1/?file=1194999#file1194999line1245> > > > > How about `allocatedStage2`? Good, much shorter with the same meaning. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42221/#review114886 --- On Jan. 12, 2016, 11:06 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42221/ > --- > > (Updated Jan. 12, 2016, 11:06 p.m.) > > > Review request for mesos, Ben Mahler and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > Since wDRF is encapsulated in the choice of sorter, it should not be > mentioned in the allocator code. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f > > Diff: https://reviews.apache.org/r/42221/diff/ > > > Testing > --- > > `make check` on Mac OS 10.10.4 > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 42305: Updated comments around sorters in the allocator.
> On Jan. 16, 2016, 11:09 p.m., Joris Van Remoortere wrote: > > src/master/allocator/mesos/hierarchical.hpp, line 412 > > <https://reviews.apache.org/r/42305/diff/2/?file=1196926#file1196926line412> > > > > I think you mean `Level 1`? Correct, sorry! - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42305/#review114890 --- On Jan. 14, 2016, 3:39 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42305/ > --- > > (Updated Jan. 14, 2016, 3:39 p.m.) > > > Review request for mesos, Ben Mahler and Joris Van Remoortere. > > > Bugs: MESOS-4128 > https://issues.apache.org/jira/browse/MESOS-4128 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.hpp > 5f08b6e7d18766d7d0f8350a280ec577b4895fb9 > > Diff: https://reviews.apache.org/r/42305/diff/ > > > Testing > --- > > make check on Mac OS 10.10.4 > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 42222: Added a comment on allocator recovery.
> On Jan. 15, 2016, 6:50 a.m., Ben Mahler wrote: > > Thanks Alex! I ended up going over the structure of the recover code and > > left some higher level comments. There also appears to be a bug that will > > crash the master that I marked as an issue :) > > > > Have we convinced ourselves that we want to act on partial state when quota > > is *not* set? It seems to complicate things here, and acting on partial > > state w/o quota has some issues as well? Thanks a bunch for a thorough review, Ben! I have opened a separate ticket to track the bug and other comments you had left: https://issues.apache.org/jira/browse/MESOS-4417. > On Jan. 15, 2016, 6:50 a.m., Ben Mahler wrote: > > src/master/allocator/mesos/hierarchical.cpp, lines 205-206 > > <https://reviews.apache.org/r/4/diff/1/?file=1195000#file1195000line205> > > > > It looks like if we trip the `resume` call in `addSlave`, this delayed > > resume will crash the master due to the `CHECK(paused)` that currently > > resides in `resume`. > > > > Something I'm missing? > > Joris Van Remoortere wrote: > Thanks for your review Ben! > I'm committing just the comment change for this patch. > I'll leave this issue open as we want to address it in a separate patch. Filed: https://issues.apache.org/jira/browse/MESOS-4417 - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42222/#review114628 --- On Jan. 12, 2016, 11:06 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4/ > --- > > (Updated Jan. 12, 2016, 11:06 p.m.) > > > Review request for mesos, Ben Mahler and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > df8bccaf2b8cfc0cb5ca18d4867371ae7a84c12f > > Diff: https://reviews.apache.org/r/4/diff/ > > > Testing > --- > > None: not a functional change. > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 42476: Introduced protobuf for set quota requests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42476/ --- (Updated Jan. 18, 2016, 11:37 p.m.) Review request for mesos, Ben Mahler, Joerg Schad, and Joris Van Remoortere. Changes --- Rebased. Bugs: MESOS-4410 https://issues.apache.org/jira/browse/MESOS-4410 Repository: mesos Description --- See summary. Diffs (updated) - docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79 src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36 Diff: https://reviews.apache.org/r/42476/diff/ Testing --- make check on Mac OS 10.10.4 Thanks, Alexander Rukletsov
Review Request 42477: Corrected example in quota user doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42477/ --- Review request for mesos, Joerg Schad and Joris Van Remoortere. Repository: mesos Description --- See summary. Diffs - docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 Diff: https://reviews.apache.org/r/42477/diff/ Testing --- None: Not a functional change. Thanks, Alexander Rukletsov
Re: Review Request 42289: Quota doesn't allocate resources on slave joining.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42289/#review115132 --- Let's tweak some wording and testing and we are good to go! I liked the initial summary more. IMO a patch should describe the solution, and not the problem. It's quite opposite for JIRA tickets, hence I'm convinced patches and tickets should not share the same title. How about "Calcuated 'remainingClusterResources' using all available agents." I also think it won't hurt to extend the description and explain why we need this change. How about "Event-triggered allocations do not include all available agents. If we calculate remaining resources in the cluster using the partial view, we may overlook already laid away resources for quota'ed roles and lay away more. Hence we may unnecessarily deprive non-quota'ed frameworks of resources." Changes touching allocator are vulnerable to races, especially in tests. Please extend the testing (and mention this in the "Testing Done" section) with goodies like `GTEST_FILTER="*HierarchicalAllocatorTest*" ./bin/mesos-tests.sh --gtest_shuffle --gtest-repeat`. Let's add a test for this change. Ideally the test should fail without the change and pass it with the change. I think Neil has already provided the test in the ticket, could you please include it? src/master/allocator/mesos/hierarchical.cpp (lines 1227 - 1228) <https://reviews.apache.org/r/42289/#comment175992> Let's add a note here that we iterate over all slaves and why. How about // NOTE: We use all active agents and not just those visible in the current `allocate()` call so that frameworks in roles without quota are not unnecessarily deprived of resources. - Alexander Rukletsov On Jan. 19, 2016, 8:50 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42289/ > --- > > (Updated Jan. 19, 2016, 8:50 a.m.) > > > Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van > Remoortere, and Neil Conway. > > > Bugs: MESOS-4102 > https://issues.apache.org/jira/browse/MESOS-4102 > > > Repository: mesos > > > Description > --- > > Calcuated 'remainingClusterResources' by all activated slaves. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > 48acde69b1a2f305b568a7e322a58708063dd30a > > Diff: https://reviews.apache.org/r/42289/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 42289: Quota doesn't allocate resources on slave joining.
> On Jan. 19, 2016, 9:23 a.m., Alexander Rukletsov wrote: > > Let's tweak some wording and testing and we are good to go! > > > > I liked the initial summary more. IMO a patch should describe the solution, > > and not the problem. It's quite opposite for JIRA tickets, hence I'm > > convinced patches and tickets should not share the same title. How about > > "Calcuated 'remainingClusterResources' using all available agents." > > > > I also think it won't hurt to extend the description and explain why we > > need this change. How about > > "Event-triggered allocations do not include all available agents. If we > > calculate remaining resources in the cluster using the partial view, > > we may overlook already laid away resources for quota'ed roles and lay > > away more. Hence we may unnecessarily deprive non-quota'ed frameworks > > of resources." > > > > Changes touching allocator are vulnerable to races, especially in tests. > > Please extend the testing (and mention this in the "Testing Done" section) > > with goodies like `GTEST_FILTER="*HierarchicalAllocatorTest*" > > ./bin/mesos-tests.sh --gtest_shuffle --gtest-repeat`. > > > > Let's add a test for this change. Ideally the test should fail without the > > change and pass it with the change. I think Neil has already provided the > > test in the ticket, could you please include it? Maybe even better, you can modify existing `QuotaAbsentFramework` test. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42289/#review115132 --- On Jan. 19, 2016, 8:50 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42289/ > --- > > (Updated Jan. 19, 2016, 8:50 a.m.) > > > Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van > Remoortere, and Neil Conway. > > > Bugs: MESOS-4102 > https://issues.apache.org/jira/browse/MESOS-4102 > > > Repository: mesos > > > Description > --- > > Calcuated 'remainingClusterResources' by all activated slaves. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > 48acde69b1a2f305b568a7e322a58708063dd30a > > Diff: https://reviews.apache.org/r/42289/diff/ > > > Testing > --- > > make > make check > > > Thanks, > > Klaus Ma > >
Re: Review Request 41769: Made allocator traverse all roles for quota allocation.
te, please move it before recover section. Also, if you do not expect any allocations for addSalve, please explain why. src/tests/hierarchical_allocator_tests.cpp (line 1941) <https://reviews.apache.org/r/41769/#comment176022> Let's describe the cluster state here. Example from one of the existing tests: ``` // Total cluster resources (1 agent): cpus=1, mem=512. // QUOTA_ROLE share = 0.25 (cpus=0.25, mem=128) [quota: cpus=0.25, mem=128] // framework1 share = 1 // NO_QUOTA_ROLE share = 0.75 (cpus=0.75, mem=384) // framework2 share = 0 ``` src/tests/hierarchical_allocator_tests.cpp (lines 1957 - 1958) <https://reviews.apache.org/r/41769/#comment176023> We need explain why. - Alexander Rukletsov On Jan. 19, 2016, 10 a.m., Guangya Liu wrote: > > ------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41769/ > --- > > (Updated Jan. 19, 2016, 10 a.m.) > > > Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, Klaus > Ma, and Neil Conway. > > > Bugs: MESOS-4411 > https://issues.apache.org/jira/browse/MESOS-4411 > > > Repository: mesos > > > Description > --- > > This patch include two parts: > 1) If there are some `non-active roles` in front of active roles after > `quotaRoleSorter`, when the allocator encounter a `non-active role`, the > allocator should not `break` but `continue` to allocate Quota for other > active roles to make sure other roles can get its quotaed resources. > 2) If some role's quota reach its guaranteed value, the allocator should > handle another role but not break. Take the following case: role1 has quota 5 > and got 5, role2 has quota 100 and got 50, the role1 will be put in front of > role2 by the `quotaRoleSorter`, if allocator `break` when found role1 is > satisfied, then role2 will never get its quotaed resources. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > 72e69a0f42dd724713f2a7a75f1b92ef16eb5569 > src/tests/hierarchical_allocator_tests.cpp > 9362dd306497ba01e0f387c3862456cdcac6f863 > > Diff: https://reviews.apache.org/r/41769/diff/ > > > Testing > --- > > make > make check > GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" > --verbose --gtest_repeat=100 --gtest_shuffle > > > Thanks, > > Guangya Liu > >
Re: Review Request 42476: Introduced protobuf for set quota requests.
> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote: > > src/master/quota_handler.cpp, line 280 > > <https://reviews.apache.org/r/42476/diff/2/?file=1200760#file1200760line280> > > > > Not an issue, but question/remark: the protoRequest.error() is most > > likely to be less concise compared to the previous error messages? Do you think we should hide the real error from an operator? I'd rather have the precise error which an operator can show me on irc or user list : ). > On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote: > > include/mesos/quota/quota.proto, line 54 > > <https://reviews.apache.org/r/42476/diff/2/?file=1200759#file1200759line54> > > > > Given the naming scheme QuotaStatus wouldn't a more consistent name be > > QuotaSet? The question here is whether we will be reusing the same protobuf for, say, update requests? My feeling is that this should be possible, hence a more general naming. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42476/#review115127 ------- On Jan. 18, 2016, 11:37 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42476/ > --- > > (Updated Jan. 18, 2016, 11:37 p.m.) > > > Review request for mesos, Ben Mahler, Joerg Schad, and Joris Van Remoortere. > > > Bugs: MESOS-4410 > https://issues.apache.org/jira/browse/MESOS-4410 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6 > include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79 > src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d > src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d > src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36 > > Diff: https://reviews.apache.org/r/42476/diff/ > > > Testing > --- > > make check on Mac OS 10.10.4 > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 42289: Calcuated 'remainingClusterResources' by all activated slaves.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42289/#review115142 --- It would be great if you add a comment describing the cluster state, have a look at other tests for an example! src/master/allocator/mesos/hierarchical.cpp (line 1230) <https://reviews.apache.org/r/42289/#comment176026> Backtick allocate() please. src/tests/hierarchical_allocator_tests.cpp (lines 1823 - 1824) <https://reviews.apache.org/r/42289/#comment176028> Let's add a `NOTE:` here that each `addSlave()` triggers an event-based allocation. src/tests/hierarchical_allocator_tests.cpp (lines 1826 - 1828) <https://reviews.apache.org/r/42289/#comment176027> Any reason why you change the original comment? - Alexander Rukletsov On Jan. 19, 2016, 11:10 a.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42289/ > --- > > (Updated Jan. 19, 2016, 11:10 a.m.) > > > Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van > Remoortere, and Neil Conway. > > > Bugs: MESOS-4102 > https://issues.apache.org/jira/browse/MESOS-4102 > > > Repository: mesos > > > Description > --- > > __Phenomenon__: > Quota doesn't allocate resources on slave joining. > > __Root Cause__: > Event-triggered allocations do not include all available agents. If we > calculate remaining resources in the cluster using the partial view, > we may overlook already laid away resources for quota'ed roles and lay > away more. Hence we may unnecessarily deprive non-quota'ed frameworks > of resources. > > Refer to AlexR's comments for more detail: > https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495 > > __Solution/Fix__: > Calcuated 'remainingClusterResources' by all activated slaves. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > 48acde69b1a2f305b568a7e322a58708063dd30a > src/tests/hierarchical_allocator_tests.cpp > 9362dd306497ba01e0f387c3862456cdcac6f863 > > Diff: https://reviews.apache.org/r/42289/diff/ > > > Testing > --- > > make > make check > ./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose > --gtest_repeat=100 --gtest_shuffle > > > Thanks, > > Klaus Ma > >
