Re: Review Request 32398: Persisted the reservation state on the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32398/#review83609 --- Ship it! Ship It! - Alexander Rukletsov On May 12, 2015, 6:44 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32398/ --- (Updated May 12, 2015, 6:44 p.m.) Review request for mesos, Alexander Rukletsov and Jie Yu. Bugs: MESOS-2491 https://issues.apache.org/jira/browse/MESOS-2491 Repository: mesos Description --- * Added `isDynamicallyReserved(resource)` which returns true if the resource is a dynamic reservation. * Added the `isDynamicallyReserved` condition onto `needCheckpointing`. * Updated the `applyCheckpointedResources` to consider dynamic reservations. * Added tests. Diffs - include/mesos/resources.hpp 4c036d36e0e8ab3852786dd32b2d983d45891624 src/common/resources.cpp 235930ff2dbb3ea49a3a0696dc070f2bd56fba4b src/common/resources_utils.cpp fe04d57227fa193d6d11d2f76529c46aea74c6a1 src/tests/reservation_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32398/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 32398: Persisted the reservation state on the slave.
On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
src/common/resources.cpp, lines 442-445
https://reviews.apache.org/r/32398/diff/2/?file=921003#file921003line442
Let's use this function where we already check for this condition, like
in `master/validation.cpp` in
```
OptionError validateDynamicReservation(
const RepeatedPtrFieldResource resources)
```
Michael Park wrote:
I used the same pattern that exists for `isPersistentVolume` and
`validatePersistentVolume`. `validatePersistentVolume` essentially repeats
the checks that are performed in `isPersistentVolume`:
```cpp
bool Resources::isPersistentVolume(const Resource resource)
{
return resource.has_disk() resource.disk().has_persistence();
}
```
```cpp
// Validates that all the given resources are persistent volumes.
OptionError validatePersistentVolume(
const RepeatedPtrFieldResource volumes)
{
foreach (const Resource volume, volumes) {
if (!volume.has_disk()) {
return Error(Resource + stringify(volume) + does not have
DiskInfo);
} else if (!volume.disk().has_persistence()) {
return Error('persistence' is not set in DiskInfo);
}
}
return None();
}
```
This was discussed with BenM and Jie, and the conclusion was that since
this is validation code, we should try to provide detailed error messages
rather than simply not a persistent volume.
I think the logic could be leveraged the other way around, where
`isPersistentVolume` would simply call: `return
validatePersistentVolume(resource).isNone();` but that means
`validatePersistentVolume` would need to live in `resources.cpp` rather than
in `master.cpp` :(
I see. Better error messages seem to be a reasonable price for this. Dropping.
On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
src/common/resources_utils.cpp, lines 45-46
https://reviews.apache.org/r/32398/diff/2/?file=921004#file921004line45
I like the name a lot, gives a good understanding on what's going to
happen, but let's still leave a comment on what we going to achieve with
stripping and why we need it.
Michael Park wrote:
I think this comment was addressed when I addressed Jie's. Could you
confirm that the comment I added is satisfactory?
Yep, good enough.
On April 24, 2015, 8:18 a.m., Alexander Rukletsov wrote:
src/tests/reservation_tests.cpp, lines 639-646
https://reviews.apache.org/r/32398/diff/2/?file=921005#file921005line639
What is the purpose of using the reverse order here?
Michael Park wrote:
It's becuase the `FUTURE_PROTOBUF` has a `EXPECT_CALL` hiding inside of
it, and the expectations are satisfied in reverse order in `gmock`.
From [Using Multiple
Expectations](https://code.google.com/p/googlemock/wiki/ForDummies#Using_Multiple_Expectations):
By default, when a mock method is invoked, Google Mock will search the
expectations in the reverse order they are defined, and stop when an active
expectation that matches the arguments is found (you can think of it as
newer rules override older ones.)
Didn't know that, good to know, thanks!
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32398/#review81456
---
On May 12, 2015, 6:44 p.m., Michael Park wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32398/
---
(Updated May 12, 2015, 6:44 p.m.)
Review request for mesos, Alexander Rukletsov and Jie Yu.
Bugs: MESOS-2491
https://issues.apache.org/jira/browse/MESOS-2491
Repository: mesos
Description
---
* Added `isDynamicallyReserved(resource)` which returns true if the resource
is a dynamic reservation.
* Added the `isDynamicallyReserved` condition onto `needCheckpointing`.
* Updated the `applyCheckpointedResources` to consider dynamic reservations.
* Added tests.
Diffs
-
include/mesos/resources.hpp 4c036d36e0e8ab3852786dd32b2d983d45891624
src/common/resources.cpp 235930ff2dbb3ea49a3a0696dc070f2bd56fba4b
src/common/resources_utils.cpp fe04d57227fa193d6d11d2f76529c46aea74c6a1
src/tests/reservation_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/32398/diff/
Testing
---
make check
Thanks,
Michael Park
Re: Review Request 29947: Fixed a race condition in hook tests for remove-executor hook.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29947/ --- (Updated May 13, 2015, 4:36 p.m.) Review request for mesos and Niklas Nielsen. Changes --- Updated the diff to use a libprocess event instead of promise pointers. Bugs: MESOS-2226 https://issues.apache.org/jira/browse/MESOS-2226 Repository: mesos Description --- There is currently no good way to synchronize between the test body and the hook code, so we wire a promise (owned by the test code). The technical debt is covered by the following JIRA issue: https://issues.apache.org/jira/browse/MESOS-2641 Diffs (updated) - src/examples/test_hook_module.cpp b25830ab6475f997422cfd2f60cc9a79e1acadfe src/messages/messages.proto 98d859f3db6013a2e155d838f590a0cde6dc5ed5 src/tests/hook_tests.cpp a65c0ab7c41ec3b7964f9d572381fa3e61746dc3 Diff: https://reviews.apache.org/r/29947/diff/ Testing --- make check Thanks, Kapil Arya
Re: Review Request 30774: Fetcher Cache
On April 29, 2015, 3:41 p.m., Benjamin Hindman wrote:
src/slave/containerizer/fetcher.cpp, lines 408-417
https://reviews.apache.org/r/30774/diff/44/?file=945694#file945694line408
For the future:
auto futures = filter(entries, [](const auto entry) { return
entry.isSome() ? entry.get() : None(); });
;-)
I could not find a suitable filter function in std, boost, or stout yet. Shall
we create one?
- Bernd
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82017
---
On May 12, 2015, 3:43 p.m., Bernd Mathiske wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
---
(Updated May 12, 2015, 3:43 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and
Timothy Chen.
Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and
MESOS-2074
https://issues.apache.org/jira/browse/MESOS-2057
https://issues.apache.org/jira/browse/MESOS-2069
https://issues.apache.org/jira/browse/MESOS-2070
https://issues.apache.org/jira/browse/MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2073
https://issues.apache.org/jira/browse/MESOS-2074
Repository: mesos
Description
---
Almost all of the functionality in epic MESOS-336. Downloaded files from
CommandInfo::URIs can now be cached in a cache directory designated by a
slave flag. This only happens when asked for by an extra flag in the URI and
is thus backwards-compatible. The cache has a size limit also given by a new
slave flag. Cache-resident files are evicted as necessary to make space for
newly fetched ones. Concurrent attempts to cache the same URI leads to only
one download. The fetcher program remains external for safety reasons, but is
now augmented with more elaborate parameters packed into a JSON object to
implement specific fetch actions for all of the above. Additional testing
includes fetching from (mock) HDFS and coverage of the new features.
Diffs
-
docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b
docs/fetcher-cache-internals.md PRE-CREATION
docs/fetcher.md PRE-CREATION
include/mesos/fetcher/fetcher.proto
311af9aebc6a85dadba9dbeffcf7036b70896bcc
include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f
include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b
src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25
src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec
src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f
src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6
src/slave/containerizer/fetcher.hpp
1db0eaf002c8d0eaf4e0391858e61e0912b35829
src/slave/containerizer/fetcher.cpp
9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2
src/slave/containerizer/mesos/containerizer.hpp
5e5f13ed8a71ff9510b40b6032d00fd16d312622
src/slave/containerizer/mesos/containerizer.cpp
f2587280dc0e1d566d2b856a80358c7b3896c603
src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13
src/tests/docker_containerizer_tests.cpp
c9d66b3fbc7d081f36c26781573dca50de823c44
src/tests/fetcher_cache_tests.cpp PRE-CREATION
src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e
src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d
src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264
Diff: https://reviews.apache.org/r/30774/diff/
Testing
---
make check
--- longer Description: ---
-Replaces all other reviews for the fetcher cache except those related to
stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616,
30618, 30621, 30626. See descriptions of those. In dependency order:
30033: Removes the fetcher env tests since these won't be needed any more
when the fetcher uses JSON in a single env var as a parameter. They never
tested anything that won't be covered by other tests anyway.
30034: Makes the code structure of all fetcher tests the same. Instead of
calling the run method of the fetcher directly, calling through fetch(). Also
removes all uses of I/O redirection, which is not really needed for
debugging, and thus the next patch can refactor fetch() and run(). (The
latter comes in two varieties, which complicates matters without much
benefit.)
30036: Extends the
Re: Review Request 32982: Added reservation user guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32982/ --- (Updated May 13, 2015, 10:15 p.m.) Review request for mesos, Alexander Rukletsov, Jie Yu, and Timothy Chen. Changes --- Updated the description with the current state of this review request. Bugs: MESOS-2205 https://issues.apache.org/jira/browse/MESOS-2205 Repository: mesos Description (updated) --- See summary. NOTE: The framework API should be reviewed thoroughly at this point since those have been implemented and landed. The master API endpoints however are still in development and therefore the state reflected in this patch is incomplete. (e.g. what HTTP code do we return on failures?) I'll update the guide to accurately reflect the master endpoints as it gets more solified. The purpose of including it was/is to get initial feedback on how it generally works. Diffs - docs/reservation.md PRE-CREATION Diff: https://reviews.apache.org/r/32982/diff/ Testing --- Documentation. Thanks, Michael Park
Re: Review Request 33919: Integrated resources estimator with the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33919/ --- (Updated May 14, 2015, 12:07 a.m.) Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone. Changes --- changed the constant naming. added TODO about the push model. Bugs: MESOS-2653 https://issues.apache.org/jira/browse/MESOS-2653 Repository: mesos Description --- Integrated resources estimator with the slave. This patch hooks the resources estimator with the slave. Slave will simply forward the estimation from the resources estimator to the master. Diffs (updated) - include/mesos/slave/resource_estimator.hpp PRE-CREATION src/Makefile.am 5b2c6014dde64a5f29986b375209de85187482a4 src/local/local.cpp dda25ab348c6430360c4b88e1d93dc70d14738d2 src/messages/messages.proto 98d859f3db6013a2e155d838f590a0cde6dc5ed5 src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec src/slave/constants.cpp 2a99b1105af0e2d62b956fa96988f477937a46bd src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 src/slave/main.cpp c62d3ab9825bf952071e8e312d383a0cb46547d2 src/slave/resource_estimator.hpp PRE-CREATION src/slave/resource_estimator.cpp PRE-CREATION src/slave/slave.hpp adb52b587ae151a51cf21ebd4923aaec1548ef10 src/slave/slave.cpp 1b17441d7153f25bd16e6b1d863ddca265480eb5 src/tests/cluster.hpp 95061665974a7aca98c374735dd9520278d89043 src/tests/mesos.hpp 563b833f8563ee6687ef95143815ed38dff71657 src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 src/tests/oversubscription_tests.cpp PRE-CREATION src/tests/slave_tests.cpp 04e79ec061e767a82d40242be7884967942c50b6 Diff: https://reviews.apache.org/r/33919/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 30774: Fetcher Cache
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30774/ --- (Updated May 13, 2015, 3:07 p.m.) Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen. Changes --- Changed the way waiting for task completion is handled. Moved such waiting outside of launchTask(). Made helper functions create futures we can wait for with AWAIT_READY_FOR. Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074 https://issues.apache.org/jira/browse/MESOS-2057 https://issues.apache.org/jira/browse/MESOS-2069 https://issues.apache.org/jira/browse/MESOS-2070 https://issues.apache.org/jira/browse/MESOS-2072 https://issues.apache.org/jira/browse/MESOS-2073 https://issues.apache.org/jira/browse/MESOS-2074 Repository: mesos Description --- Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features. Diffs (updated) - docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b docs/fetcher-cache-internals.md PRE-CREATION docs/fetcher.md PRE-CREATION include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 src/tests/fetcher_cache_tests.cpp PRE-CREATION src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 Diff: https://reviews.apache.org/r/30774/diff/ Testing --- make check --- longer Description: --- -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order: 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway. 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.) 30036: Extends the CommandInfo::URI protobuf with a boolean caching field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed. 30037: Extends the fetcher info protobuf with actions (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to items, which contain the uri, the action, and potentially a cache
Re: Review Request 31667: Piped hashmapSlaveID, Resources from allocator through to sorter.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31667/#review83669 --- Ship it! Looks good, just some minor notes below. Went over these with Michael so I'll make the updates for him. src/master/allocator/sorter/drf/sorter.cpp https://reviews.apache.org/r/31667/#comment134685 Hm.. since we're guaranteed to just re-insert on the next line, let's not bother with this? src/master/allocator/sorter/drf/sorter.cpp https://reviews.apache.org/r/31667/#comment134686 Maybe we should make it more clear about why we're even talking about safety in the first place? Might not be clear to the reader that Resources::sum is known to be problematic for non-scalars: ``` // NOTE: Summation is incorrect for non-scalars, but since we // only care about scalar resources, this is safe. ``` src/master/allocator/sorter/sorter.hpp https://reviews.apache.org/r/31667/#comment134680 Thanks! src/master/allocator/sorter/sorter.hpp https://reviews.apache.org/r/31667/#comment134682 #include stout/hashmap.hpp for this? src/tests/sorter_tests.cpp https://reviews.apache.org/r/31667/#comment134687 Let's break this line consistently with the one below, just to make it more readable :) src/tests/sorter_tests.cpp https://reviews.apache.org/r/31667/#comment134688 Would be nice to also validate that there is only 1 entry in the map :) src/tests/sorter_tests.cpp https://reviews.apache.org/r/31667/#comment134689 How about MultipleSlaves and UpdateMultipleSlaves? Since this is these are the only tests now with multiple slaves :) src/tests/sorter_tests.cpp https://reviews.apache.org/r/31667/#comment134690 Let's check the size too? src/tests/sorter_tests.cpp https://reviews.apache.org/r/31667/#comment134700 EXPECT_EQ wants the expected value as the first argument (to print expected vs actual), so let's flip the arguments here src/tests/sorter_tests.cpp https://reviews.apache.org/r/31667/#comment134692 This says construct an offer operation, but you just flatten? Seems like we should go through the apply() operation code here. src/tests/sorter_tests.cpp https://reviews.apache.org/r/31667/#comment134691 Let's check the size too? src/tests/sorter_tests.cpp https://reviews.apache.org/r/31667/#comment134701 Ditto here, let's flip the arguments. - Ben Mahler On May 10, 2015, 12:21 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31667/ --- (Updated May 10, 2015, 12:21 a.m.) Review request for mesos, Alexander Rukletsov and Ben Mahler. Bugs: MESOS-2373 https://issues.apache.org/jira/browse/MESOS-2373 Repository: mesos Description --- `Sorter` changes: - Augmented `add`, `remove`, `allocated`, `unallocated`, `update` with `SlaveID`. - `allocation` returns `hashmapSlaveID, Resources`. `DRFSorter` changes: - `allocations` is updated from `hashmapstd::string, Resources` to `hashmapstd::string, hashmapSlaveID, Resources`. - `resources` is updated from `Resources` to `hashmapSlaveID, Resources`. Diffs - src/master/allocator/mesos/hierarchical.hpp 09adced9d8712b3eeda885d598443791186890db src/master/allocator/sorter/drf/sorter.hpp 4366710d6530b784aa5094813328d0e338239ba0 src/master/allocator/sorter/drf/sorter.cpp 2f69f384b95ff20d3ee429a4570a8cffa74d8e8b src/master/allocator/sorter/sorter.hpp e2efb27b11dbea42dd73f81e5db0d6d2b0a6034b src/tests/sorter_tests.cpp 42442353afe7bd3d1a5b43992f8ae191ac19bdcd Diff: https://reviews.apache.org/r/31667/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 32982: [WIP] Added reservation user guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32982/#review83678 --- docs/reservation.md https://reviews.apache.org/r/32982/#comment134702 Simpler: No breaking changes are introduced with dynamic reservation, ... Suggestion: singular reservation here. Either this or make the mention in line 7 plural as well. docs/reservation.md https://reviews.apache.org/r/32982/#comment134703 s/manage the/manage There are no definite reservations this sentence refers to. docs/reservation.md https://reviews.apache.org/r/32982/#comment134704 s/we'll/we will docs/reservation.md https://reviews.apache.org/r/32982/#comment134706 s/we'll/we will and please check other places Suggestion: avoid colloquial style in documentation. docs/reservation.md https://reviews.apache.org/r/32982/#comment134708 This sentence does not strictly add any information. It is more likely confusing. What does it mean to convert a resource? I suppose we are merely accounting for them here, not physically affecting the resources themselves, right? Suggestion: The Mesos master will take the requested reservation into account. docs/reservation.md https://reviews.apache.org/r/32982/#comment134710 Same as the above sentence. docs/reservation.md https://reviews.apache.org/r/32982/#comment134711 state of the recources - what does this mean? Suggestion: resource specifications docs/reservation.md https://reviews.apache.org/r/32982/#comment134712 s/an/a docs/reservation.md https://reviews.apache.org/r/32982/#comment134713 Isn't this the exact same offer as above? Can we simply reference it instead of repeating it? docs/reservation.md https://reviews.apache.org/r/32982/#comment134714 Even if we repeat the offer from way above here, we may want to mention that it is exactly the same again. - Bernd Mathiske On May 13, 2015, 3:15 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32982/ --- (Updated May 13, 2015, 3:15 p.m.) Review request for mesos, Alexander Rukletsov, Jie Yu, and Timothy Chen. Bugs: MESOS-2205 https://issues.apache.org/jira/browse/MESOS-2205 Repository: mesos Description --- See summary. NOTE: The framework API should be reviewed thoroughly at this point since those have been implemented and landed. The master API endpoints however are still in development and therefore the state reflected in this patch is incomplete. (e.g. what HTTP code do we return on failures?) I'll update the guide to accurately reflect the master endpoints as it gets more solified. The purpose of including it was/is to get initial feedback on how it generally works. Diffs - docs/reservation.md PRE-CREATION Diff: https://reviews.apache.org/r/32982/diff/ Testing --- Documentation. Thanks, Michael Park
Re: Review Request 33919: Integrated resources estimator with the slave.
On May 13, 2015, 5:24 p.m., Niklas Nielsen wrote: src/slave/flags.cpp, line 443 https://reviews.apache.org/r/33919/diff/2/?file=957302#file957302line443 Don't we usually use an empty string instead of a placeholder? See for example hadoop_home Changed it to Option instead. On May 13, 2015, 5:24 p.m., Niklas Nielsen wrote: src/slave/resource_estimator.hpp, lines 33-34 https://reviews.apache.org/r/33919/diff/2/?file=957304#file957304line33 Shouldn't we follow the factory-ish pattern we have used with: Containerizer::create() Allocator::create() ... :) This is tricky because ResourceEstimator is exposed for modules. We need slave flags for this function and it's not exposed. On May 13, 2015, 5:24 p.m., Niklas Nielsen wrote: src/slave/resource_estimator.cpp, line 38 https://reviews.apache.org/r/33919/diff/2/?file=957305#file957305line38 What does 'EmptyResourceEstimator' mean? Is it a NOOP one? A no-logic, measured slack to resources estimator? I think we could name it a bit more precisely. Renamed it to Noop. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33919/#review83630 --- On May 13, 2015, 12:37 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33919/ --- (Updated May 13, 2015, 12:37 a.m.) Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2653 https://issues.apache.org/jira/browse/MESOS-2653 Repository: mesos Description --- Integrated resources estimator with the slave. This patch hooks the resources estimator with the slave. Slave will simply forward the estimation from the resources estimator to the master. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/local/local.cpp dda25ab348c6430360c4b88e1d93dc70d14738d2 src/messages/messages.proto 98d859f3db6013a2e155d838f590a0cde6dc5ed5 src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec src/slave/constants.cpp 2a99b1105af0e2d62b956fa96988f477937a46bd src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 src/slave/main.cpp c62d3ab9825bf952071e8e312d383a0cb46547d2 src/slave/resource_estimator.hpp PRE-CREATION src/slave/resource_estimator.cpp PRE-CREATION src/slave/slave.hpp adb52b587ae151a51cf21ebd4923aaec1548ef10 src/slave/slave.cpp 1b17441d7153f25bd16e6b1d863ddca265480eb5 src/tests/cluster.hpp 95061665974a7aca98c374735dd9520278d89043 src/tests/mesos.hpp 563b833f8563ee6687ef95143815ed38dff71657 src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 src/tests/oversubscription_tests.cpp PRE-CREATION src/tests/slave_tests.cpp 04e79ec061e767a82d40242be7884967942c50b6 Diff: https://reviews.apache.org/r/33919/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 33058: Updated test-frameworks to support principal only credential.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33058/#review83683 --- src/examples/java/TestFramework.java https://reviews.apache.org/r/33058/#comment134716 line 80 chars - Bernd Mathiske On April 10, 2015, 12:25 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33058/ --- (Updated April 10, 2015, 12:25 a.m.) Review request for mesos, Adam B and Alexander Rojas. Bugs: MESOS-2608 https://issues.apache.org/jira/browse/MESOS-2608 Repository: mesos-incubating Description --- see summary. Diffs - src/examples/java/TestFramework.java 9e95369 src/examples/python/test_framework.py a179df5 src/examples/test_framework.cpp 9f4b53e Diff: https://reviews.apache.org/r/33058/diff/ Testing --- make check + functional testing using custom authentication modules not relying on a shared secrets. Thanks, Till Toenshoff
Re: Review Request 33919: Integrated resources estimator with the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33919/#review83691 --- Patch looks great! Reviews applied: [33918, 33919] All tests passed. - Mesos ReviewBot On May 14, 2015, 12:07 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33919/ --- (Updated May 14, 2015, 12:07 a.m.) Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2653 https://issues.apache.org/jira/browse/MESOS-2653 Repository: mesos Description --- Integrated resources estimator with the slave. This patch hooks the resources estimator with the slave. Slave will simply forward the estimation from the resources estimator to the master. Diffs - include/mesos/slave/resource_estimator.hpp PRE-CREATION src/Makefile.am 5b2c6014dde64a5f29986b375209de85187482a4 src/local/local.cpp dda25ab348c6430360c4b88e1d93dc70d14738d2 src/messages/messages.proto 98d859f3db6013a2e155d838f590a0cde6dc5ed5 src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec src/slave/constants.cpp 2a99b1105af0e2d62b956fa96988f477937a46bd src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 src/slave/main.cpp c62d3ab9825bf952071e8e312d383a0cb46547d2 src/slave/resource_estimator.hpp PRE-CREATION src/slave/resource_estimator.cpp PRE-CREATION src/slave/slave.hpp adb52b587ae151a51cf21ebd4923aaec1548ef10 src/slave/slave.cpp 1b17441d7153f25bd16e6b1d863ddca265480eb5 src/tests/cluster.hpp 95061665974a7aca98c374735dd9520278d89043 src/tests/mesos.hpp 563b833f8563ee6687ef95143815ed38dff71657 src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 src/tests/oversubscription_tests.cpp PRE-CREATION src/tests/slave_tests.cpp 04e79ec061e767a82d40242be7884967942c50b6 Diff: https://reviews.apache.org/r/33919/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 33919: Integrated resources estimator with the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33919/ --- (Updated May 13, 2015, 10:36 p.m.) Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone. Changes --- s/Empty/Noop/ and changed the --resoure_estimator flag to be optional. Bugs: MESOS-2653 https://issues.apache.org/jira/browse/MESOS-2653 Repository: mesos Description --- Integrated resources estimator with the slave. This patch hooks the resources estimator with the slave. Slave will simply forward the estimation from the resources estimator to the master. Diffs (updated) - src/Makefile.am 5b2c6014dde64a5f29986b375209de85187482a4 src/local/local.cpp dda25ab348c6430360c4b88e1d93dc70d14738d2 src/messages/messages.proto 98d859f3db6013a2e155d838f590a0cde6dc5ed5 src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec src/slave/constants.cpp 2a99b1105af0e2d62b956fa96988f477937a46bd src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 src/slave/main.cpp c62d3ab9825bf952071e8e312d383a0cb46547d2 src/slave/resource_estimator.hpp PRE-CREATION src/slave/resource_estimator.cpp PRE-CREATION src/slave/slave.hpp adb52b587ae151a51cf21ebd4923aaec1548ef10 src/slave/slave.cpp 1b17441d7153f25bd16e6b1d863ddca265480eb5 src/tests/cluster.hpp 95061665974a7aca98c374735dd9520278d89043 src/tests/mesos.hpp 563b833f8563ee6687ef95143815ed38dff71657 src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 src/tests/oversubscription_tests.cpp PRE-CREATION src/tests/slave_tests.cpp 04e79ec061e767a82d40242be7884967942c50b6 Diff: https://reviews.apache.org/r/33919/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 33918: Added resources estimator abstraction for oversubscription.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33918/ --- (Updated May 13, 2015, 10:35 p.m.) Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone. Changes --- Renamed EmptyResourceEstimator to NoopResourceEstimator Bugs: MESOS-2649 https://issues.apache.org/jira/browse/MESOS-2649 Repository: mesos Description --- Added resources estimator abstraction for oversubscription. This patch defines the interface of the resources estimator and creates a default stub implementation. Diffs (updated) - include/mesos/slave/resource_estimator.hpp PRE-CREATION src/Makefile.am 5b2c6014dde64a5f29986b375209de85187482a4 src/slave/resource_estimator.hpp PRE-CREATION src/slave/resource_estimator.cpp PRE-CREATION Diff: https://reviews.apache.org/r/33918/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 33919: Integrated resources estimator with the slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33919/#review83700 --- Ship it! src/slave/resource_estimator.hpp https://reviews.apache.org/r/33919/#comment134742 Do you still need this include? src/slave/resource_estimator.cpp https://reviews.apache.org/r/33919/#comment134745 You don't need an else block here src/slave/slave.hpp https://reviews.apache.org/r/33919/#comment134746 Let's make these private (if possible) :) - Niklas Nielsen On May 13, 2015, 5:07 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33919/ --- (Updated May 13, 2015, 5:07 p.m.) Review request for mesos, Ben Mahler, Joris Van Remoortere, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2653 https://issues.apache.org/jira/browse/MESOS-2653 Repository: mesos Description --- Integrated resources estimator with the slave. This patch hooks the resources estimator with the slave. Slave will simply forward the estimation from the resources estimator to the master. Diffs - include/mesos/slave/resource_estimator.hpp PRE-CREATION src/Makefile.am 5b2c6014dde64a5f29986b375209de85187482a4 src/local/local.cpp dda25ab348c6430360c4b88e1d93dc70d14738d2 src/messages/messages.proto 98d859f3db6013a2e155d838f590a0cde6dc5ed5 src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec src/slave/constants.cpp 2a99b1105af0e2d62b956fa96988f477937a46bd src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 src/slave/main.cpp c62d3ab9825bf952071e8e312d383a0cb46547d2 src/slave/resource_estimator.hpp PRE-CREATION src/slave/resource_estimator.cpp PRE-CREATION src/slave/slave.hpp adb52b587ae151a51cf21ebd4923aaec1548ef10 src/slave/slave.cpp 1b17441d7153f25bd16e6b1d863ddca265480eb5 src/tests/cluster.hpp 95061665974a7aca98c374735dd9520278d89043 src/tests/mesos.hpp 563b833f8563ee6687ef95143815ed38dff71657 src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 src/tests/oversubscription_tests.cpp PRE-CREATION src/tests/slave_tests.cpp 04e79ec061e767a82d40242be7884967942c50b6 Diff: https://reviews.apache.org/r/33919/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 34016: Change the type of signaledWrapper to unique_ptr
On May 11, 2015, 10:19 p.m., Ben Mahler wrote: src/slave/slave.cpp, line 155 https://reviews.apache.org/r/34016/diff/1/?file=954524#file954524line155 unique_ptr is not a POD, so this will still try to run the destructor of the function during exit of the program. Can you just use a raw pointer here and leak it? (Not ideal but we use this in many places) haosdent huang wrote: Yes, I use raw pointer before. But got this error ``` ../../src/slave/slave.cpp:519:19: error: cannot convert ‘process::_Deferredstd::_Bindstd::_Mem_fnvoid (std::functionvoid(int, int)::*)(int, int)const(std::functionvoid(int, int), std::_Placeholder1, std::_Placeholder2) ’ to ‘std::functionvoid(int, int)*’ in assignment signaledWrapper = defer(self(), Slave::signaled, lambda::_1, lambda::_2); ``` So I use unique_ptr. Let me remove unique_ptr and add force convert here. haosdent huang wrote: But still have this error. ``` ../../src/slave/slave.cpp:520:55: error: taking address of temporary [-fpermissive] self(), Slave::signaled, lambda::_1, lambda::_2); ^ ``` So I use `new` here? Ben Mahler wrote: yes, you need a copy on the heap, so `new function...(defer(...));` should work haosdent huang wrote: But this heap object would never release util Slave exit, is it OK? For now, yes. We'll get this squared up when we get to tackling [MESOS-2694](https://issues.apache.org/jira/browse/MESOS-2694). Meanwhile, there are already many offending instances which make tools like `valgrind` useless anyway. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34016/#review83299 --- On May 13, 2015, 10:49 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34016/ --- (Updated May 13, 2015, 10:49 a.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Change the type of signaledWrapper to unique_ptr Diffs - src/slave/slave.cpp bf290bfd7d9a59ce7197ce34cbd8cf42e7dd17a3 Diff: https://reviews.apache.org/r/34016/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 34152: Master flag validation now supports zookeeper
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34152/#review83639 --- src/cli/execute.cpp https://reviews.apache.org/r/34152/#comment134647 This needs to be updated. See src/slave/main.cpp for an appropriate error message. src/cli/execute.cpp https://reviews.apache.org/r/34152/#comment134648 Instead of doing it this way, I would recommend doing it the way MasterDetector::create() does the URL parsing (see src/master/detector.cpp) for consistency. - Vinod Kone On May 13, 2015, 5:50 p.m., Tom Arnfeld wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34152/ --- (Updated May 13, 2015, 5:50 p.m.) Review request for mesos, Benjamin Hindman and Vinod Kone. Bugs: MESOS-2723 https://issues.apache.org/jira/browse/MESOS-2723 Repository: mesos Description --- The mesos-execute cli tool does custom validation on the --master flag to improve user error handling, but currently does not support ZooKeeper. Diffs - src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17 Diff: https://reviews.apache.org/r/34152/diff/ Testing --- Compiled and tested locally on a live cluster, with both ZooKeeper and standalone host:port. Thanks, Tom Arnfeld
Re: Review Request 33155: Added tests for slave removal metrics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33155/#review83648 --- Ship it! Ship It! - Vinod Kone On May 12, 2015, 11:32 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33155/ --- (Updated May 12, 2015, 11:32 p.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2485 https://issues.apache.org/jira/browse/MESOS-2485 Repository: mesos Description --- I updated the existing tests to validate the metrics. However, it blocks the tests due to the slave metrics never getting satisfied. Added a TODO. Diffs - src/tests/master_tests.cpp 75ffadae64ece4e3ff53abeefa5f6e8e3690d402 src/tests/partition_tests.cpp 1018e479a77a6b533f2dd392fedbdccb80e3ac1f src/tests/slave_tests.cpp 04e79ec061e767a82d40242be7884967942c50b6 Diff: https://reviews.apache.org/r/33155/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 33876: Added usages() to resource monitor
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33876/#review83645 --- Ship it! Ship It! - Jie Yu On May 12, 2015, 8:55 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33876/ --- (Updated May 12, 2015, 8:55 p.m.) Review request for mesos, Jie Yu and Vinod Kone. Repository: mesos Description --- Added usages() to resource monitor to enable internal components tapping into resource statistics. Diffs - src/slave/monitor.hpp 69c60a10187f8ea617c6be9738b28e8103e0ed27 src/slave/monitor.cpp 398af010564e999b46e38560ba1e652261a9420c Diff: https://reviews.apache.org/r/33876/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 32398: Persisted the reservation state on the slave.
On May 13, 2015, 6:04 p.m., Jie Yu wrote: I fixed a few style issues for you. Make sure the comments are wrapped at 70 chars (set tw=70). Also, this is how I used to find snake case variable names: `/[a-z]*_[a-z]*\ ` I fixed a few style issues for you. Thank you... and sorry. Make sure the comments are wrapped at 70 chars (set tw=70). The `set tw=70` should help since I'm a `vim` user myself, but I wonder if all editors have this capability. Also, this is how I used to find snake case variable names: /[a-z]*_[a-z]*\ Oh right, I just realize we actually don't use snake_case for anything in Mesos so I should be able to regex for it. I think this also means we can probably add this to our style checker. On May 13, 2015, 6:04 p.m., Jie Yu wrote: src/tests/reservation_tests.cpp, lines 1026-1027 https://reviews.apache.org/r/32398/diff/6/?file=956697#file956697line1026 :) Oh my. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32398/#review83638 --- On May 12, 2015, 6:44 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32398/ --- (Updated May 12, 2015, 6:44 p.m.) Review request for mesos, Alexander Rukletsov and Jie Yu. Bugs: MESOS-2491 https://issues.apache.org/jira/browse/MESOS-2491 Repository: mesos Description --- * Added `isDynamicallyReserved(resource)` which returns true if the resource is a dynamic reservation. * Added the `isDynamicallyReserved` condition onto `needCheckpointing`. * Updated the `applyCheckpointedResources` to consider dynamic reservations. * Added tests. Diffs - include/mesos/resources.hpp 4c036d36e0e8ab3852786dd32b2d983d45891624 src/common/resources.cpp 235930ff2dbb3ea49a3a0696dc070f2bd56fba4b src/common/resources_utils.cpp fe04d57227fa193d6d11d2f76529c46aea74c6a1 src/tests/reservation_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32398/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 34152: Master flag validation now supports zookeeper
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34152/#review83568 --- Patch looks great! Reviews applied: [34152] All tests passed. - Mesos ReviewBot On May 13, 2015, 2:08 a.m., Tom Arnfeld wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34152/ --- (Updated May 13, 2015, 2:08 a.m.) Review request for mesos. Bugs: MESOS-2723 https://issues.apache.org/jira/browse/MESOS-2723 Repository: mesos Description --- The mesos-execute cli tool does custom validation on the --master flag to improve user error handling, but currently does not support ZooKeeper. Diffs - src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17 Diff: https://reviews.apache.org/r/34152/diff/ Testing --- Compiled and tested locally on a live cluster, with both ZooKeeper and standalone host:port. Thanks, Tom Arnfeld
