Review Request 45120: Extended `os::rmdir` in stout to support preserving the root directory.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45120/
---
Review request for mesos, Jie Yu and Joris Van Remoortere.
Bugs: MESOS-2408
https://issues.apache.org/jira/browse/MESOS-2408
Repository: mesos
Description
---
This allows `os::rmdir("/x")` to optionally remove all the child
files and directories of the root directory "/x", but preserve "/x"
itself.
Diffs
-
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp
cbc97596cd8ed1e6d4261568fd0086c86e063232
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp
4e3dd0ff8cbfe470b14c7cacbd861e7dc8384ba1
3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp
daa46e05d113fd62ea9dad042ec41aaab28ad003
Diff: https://reviews.apache.org/r/45120/diff/
Testing
---
make check
Thanks,
Neil Conway
Review Request 45117: Cleaned up header includes in tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45117/ --- Review request for mesos, Jie Yu and Joris Van Remoortere. Bugs: MESOS-2408 https://issues.apache.org/jira/browse/MESOS-2408 Repository: mesos Description --- Cleaned up header includes in tests. Diffs - src/tests/containerizer/ns_tests.cpp 4bf45e970bede713fa4ffce205627b149232fe2b src/tests/http_authentication_tests.cpp cf2bb762272fa38e04e5c26aef2858300bbd0459 src/tests/persistent_volume_endpoints_tests.cpp d04063090e7f45b4c047a4e037eed1de79cd6958 src/tests/persistent_volume_tests.cpp 26fff19daa8b175fdcc06fd9467224d5920a1967 src/tests/reservation_endpoints_tests.cpp 028e28c68e8a438d310df04fea0a7e54a6d642a3 src/tests/teardown_tests.cpp a5f40f991e99ec0d452074a9d3c209b05b317f51 Diff: https://reviews.apache.org/r/45117/diff/ Testing --- make check Thanks, Neil Conway
Review Request 45119: Added TODO.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45119/ --- Review request for mesos, Jie Yu and Joris Van Remoortere. Bugs: MESOS-2408 https://issues.apache.org/jira/browse/MESOS-2408 Repository: mesos Description --- Added TODO. Diffs - src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 Diff: https://reviews.apache.org/r/45119/diff/ Testing --- make check Thanks, Neil Conway
Review Request 45118: Fixed various style issues.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45118/ --- Review request for mesos, Jie Yu and Joris Van Remoortere. Bugs: MESOS-2408 https://issues.apache.org/jira/browse/MESOS-2408 Repository: mesos Description --- Avoid capturing temporaries by const reference, which is against the style guide. Fixup some whitespace. Diffs - src/slave/state.hpp 9cc9df2f8afb2ae848db467892dfb7f54973a6a3 src/tests/persistent_volume_tests.cpp 26fff19daa8b175fdcc06fd9467224d5920a1967 Diff: https://reviews.apache.org/r/45118/diff/ Testing --- make check Thanks, Neil Conway
Review Request 45121: Implemented deletion for persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45121/ --- Review request for mesos, Jie Yu and Joris Van Remoortere. Repository: mesos Description --- Prior to this commit, destroying a persistent volume would remove the Mesos-level metadata about the volume, but wouldn't destroy any of the volume's filesystem content. We now remove the volume from the slave's filesystem, essentially via "rm -r". Diffs - src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 Diff: https://reviews.apache.org/r/45121/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 45122: Updated tests for deletion of persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45122/ --- (Updated March 21, 2016, 6:44 p.m.) Review request for mesos, Jie Yu and Joris Van Remoortere. Bugs: MESOS-2408 https://issues.apache.org/jira/browse/MESOS-2408 Repository: mesos Description --- Updated tests for deletion of persistent volumes. Diffs - src/tests/persistent_volume_tests.cpp 26fff19daa8b175fdcc06fd9467224d5920a1967 Diff: https://reviews.apache.org/r/45122/diff/ Testing --- make check Thanks, Neil Conway
Review Request 45122: Updated tests for deletion of persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45122/ --- Review request for mesos, Jie Yu and Joris Van Remoortere. Repository: mesos Description --- Updated tests for deletion of persistent volumes. Diffs - src/tests/persistent_volume_tests.cpp 26fff19daa8b175fdcc06fd9467224d5920a1967 Diff: https://reviews.apache.org/r/45122/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 45120: Extended `os::rmdir` in stout to support preserving the root directory.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45120/
---
(Updated March 26, 2016, 12:25 a.m.)
Review request for mesos, Jie Yu and Joris Van Remoortere.
Changes
---
Address review comments, rebase.
Bugs: MESOS-2408
https://issues.apache.org/jira/browse/MESOS-2408
Repository: mesos
Description
---
This allows `os::rmdir("/x")` to optionally remove all the child
files and directories of the root directory "/x", but preserve "/x"
itself.
Diffs (updated)
-
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp
44a59b830f4a09c9a19d55d3e99154ca8e4d2593
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp
4e3dd0ff8cbfe470b14c7cacbd861e7dc8384ba1
3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp
5466991675778c59d8040946717b492cdb1f9b14
Diff: https://reviews.apache.org/r/45120/diff/
Testing
---
make check
Thanks,
Neil Conway
Re: Review Request 45123: Updated docs for deletion of persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45123/ --- (Updated March 26, 2016, 12:25 a.m.) Review request for mesos, Jie Yu and Joris Van Remoortere. Changes --- Rebase. Bugs: MESOS-2408 https://issues.apache.org/jira/browse/MESOS-2408 Repository: mesos Description --- Updated docs for deletion of persistent volumes. Diffs (updated) - docs/multiple-disk.md 5cdce26721212597052515a9cf2fd9277d24e42c docs/persistent-volume.md 4b9c59daf6fdcee4a102e19d6eb4df9b5eddfa54 docs/reservation.md 55924adb94028702e15db7e191915157552981d0 docs/upgrades.md 1b683b59b981ec7e4664c751cb57ab848c7506dc Diff: https://reviews.apache.org/r/45123/diff/ Testing --- Thanks, Neil Conway
Re: Review Request 45121: Implemented deletion for persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45121/ --- (Updated March 27, 2016, 6:08 p.m.) Review request for mesos, Jie Yu and Joris Van Remoortere. Changes --- Address review comments. Bugs: MESOS-2408 https://issues.apache.org/jira/browse/MESOS-2408 Repository: mesos Description --- Prior to this commit, destroying a persistent volume would remove the Mesos-level metadata about the volume, but wouldn't destroy any of the volume's filesystem content. We now remove the volume from the slave's filesystem, essentially via "rm -r". Diffs (updated) - src/slave/slave.cpp f383605a52f31d7b805ad6153adc409dcb40f83a Diff: https://reviews.apache.org/r/45121/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 45121: Implemented deletion for persistent volumes.
> On March 26, 2016, 12:37 a.m., Jie Yu wrote: > > src/slave/slave.cpp, line 2375 > > <https://reviews.apache.org/r/45121/diff/1/?file=1308847#file1308847line2375> > > > > I would suggest we don't use CHECK here. We can just LOG(ERROR) if the > > deletion fails. Given the TODO above, we could leak disk space anyway. I used `CHECK_SOME` because we use the same error-handling strategy when creating the persistent volume fails, or if the slave fails to checkpoint resources to disk successfully. Are you sure we want to handle `rmdir` errors differently? - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45121/#review125492 --- On March 21, 2016, 6:44 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45121/ > --- > > (Updated March 21, 2016, 6:44 p.m.) > > > Review request for mesos, Jie Yu and Joris Van Remoortere. > > > Bugs: MESOS-2408 > https://issues.apache.org/jira/browse/MESOS-2408 > > > Repository: mesos > > > Description > --- > > Prior to this commit, destroying a persistent volume would remove > the Mesos-level metadata about the volume, but wouldn't destroy > any of the volume's filesystem content. We now remove the volume > from the slave's filesystem, essentially via "rm -r". > > > Diffs > - > > src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 > > Diff: https://reviews.apache.org/r/45121/diff/ > > > Testing > --- > > make check > > > Thanks, > > Neil Conway > >
Re: Review Request 45122: Updated tests for deletion of persistent volumes.
> On March 26, 2016, 12:53 a.m., Jie Yu wrote: > > src/tests/persistent_volume_tests.cpp, line 810 > > <https://reviews.apache.org/r/45122/diff/1/?file=1308848#file1308848line810> > > > > What the purpose of adding another file here? I wanted to check that if files are directly written into the directory that corresponds to the persistent volume (not via the task itself), they are still cleaned up when the volume is destroyed. > On March 26, 2016, 12:53 a.m., Jie Yu wrote: > > src/tests/persistent_volume_tests.cpp, line 388 > > <https://reviews.apache.org/r/45122/diff/1/?file=1308848#file1308848line388> > > > > Do you want to check if MOUNT disk is empty? Simplest way to do this seemed to be via `fs::list`. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45122/#review125499 ------- On March 21, 2016, 6:44 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45122/ > --- > > (Updated March 21, 2016, 6:44 p.m.) > > > Review request for mesos, Jie Yu and Joris Van Remoortere. > > > Bugs: MESOS-2408 > https://issues.apache.org/jira/browse/MESOS-2408 > > > Repository: mesos > > > Description > --- > > Updated tests for deletion of persistent volumes. > > > Diffs > - > > src/tests/persistent_volume_tests.cpp > 26fff19daa8b175fdcc06fd9467224d5920a1967 > > Diff: https://reviews.apache.org/r/45122/diff/ > > > Testing > --- > > make check > > > Thanks, > > Neil Conway > >
Re: Review Request 45122: Updated tests for deletion of persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45122/ --- (Updated March 27, 2016, 10:05 p.m.) Review request for mesos, Jie Yu and Joris Van Remoortere. Changes --- Address some review comments. Bugs: MESOS-2408 https://issues.apache.org/jira/browse/MESOS-2408 Repository: mesos Description --- Updated tests for deletion of persistent volumes. Diffs (updated) - src/tests/persistent_volume_tests.cpp 26fff19daa8b175fdcc06fd9467224d5920a1967 Diff: https://reviews.apache.org/r/45122/diff/ Testing --- make check Thanks, Neil Conway
Review Request 45038: Fixed email address in Python build file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45038/ --- Review request for mesos and Joris Van Remoortere. Repository: mesos Description --- Fixed email address in Python build file. Diffs - src/python/setup.py.in 737066952fe72382bcf80ca6d3e8457ea07a65bf Diff: https://reviews.apache.org/r/45038/diff/ Testing --- Thanks, Neil Conway
Review Request 45318: Added a comment to a utility function in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45318/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- Added a comment to a utility function in libprocess. Diffs - 3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 Diff: https://reviews.apache.org/r/45318/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 45320: Made libprocess return "Accepted" for all libprocess requests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45320/ --- (Updated March 24, 2016, 9:57 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3870 https://issues.apache.org/jira/browse/MESOS-3870 Repository: mesos Description --- Previously, we returned "Accepted" only for libprocess requests that were issued by non-libprocess clients (that is, client programs that issued requests according to the libprocess protocol but aren't themselves libprocess, such as mesos-go). The reason we didn't return "Accepted" for requests sent by libprocess is that old versions of libprocess (Mesos 0.18 and earlier) wouldn't handle such responses correctly. Since we don't need to maintain compatibility with such ancient versions of libprocess, we can now return "Accepted" responses unconditionally. Note that all recent versions of libprocess will ignore any response that is received to a libprocess request, but that will change in the near future. Diffs - 3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 3rdparty/libprocess/src/tests/process_tests.cpp 6b3aa1bcf20466cdf8f8249988b8b06dec27e5cd Diff: https://reviews.apache.org/r/45320/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 45320: Made libprocess return "Accepted" for all libprocess requests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45320/ --- (Updated March 24, 2016, 9:58 p.m.) Review request for mesos and Ben Mahler. Changes --- Tweak description. Bugs: MESOS-3870 https://issues.apache.org/jira/browse/MESOS-3870 Repository: mesos Description (updated) --- Previously, we returned "Accepted" only for libprocess requests that were issued by non-libprocess clients (that is, client programs that issued requests according to the libprocess protocol but aren't themselves libprocess, such as mesos-go). The reason we didn't return "Accepted" for requests sent by libprocess is that old versions of libprocess (Mesos 0.18 and earlier) wouldn't handle such responses correctly. Since we don't need to maintain compatibility with such ancient versions of libprocess, we can now return "Accepted" responses unconditionally. Note that all recent versions of libprocess will ignore any response that is received to a libprocess request, but that will change in the near future. Diffs (updated) - 3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 3rdparty/libprocess/src/tests/process_tests.cpp 6b3aa1bcf20466cdf8f8249988b8b06dec27e5cd Diff: https://reviews.apache.org/r/45320/diff/ Testing --- make check Thanks, Neil Conway
Review Request 45320: Made libprocess return "Accepted" for all libprocess requests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45320/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- Previously, we returned "Accepted" only for libprocess requests that were issued by non-libprocess clients (that is, client programs that issued requests according to the libprocess protocol but aren't themselves libprocess, such as mesos-go). The reason we didn't return "Accepted" for requests sent by libprocess is that old versions of libprocess (Mesos 0.18 and earlier) wouldn't handle such responses correctly. Since we don't need to maintain compatibility with such ancient versions of libprocess, we can now return "Accepted" responses unconditionally. Note that all recent versions of libprocess will ignore any response that is received to a libprocess request, but that will change in the near future. Diffs - 3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 3rdparty/libprocess/src/tests/process_tests.cpp 6b3aa1bcf20466cdf8f8249988b8b06dec27e5cd Diff: https://reviews.apache.org/r/45320/diff/ Testing --- make check Thanks, Neil Conway
Review Request 45319: Cleaned up various code in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45319/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- Cleaned up various code in libprocess. Diffs - 3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 3rdparty/libprocess/src/tests/benchmarks.cpp e8bbc55f0f9aeabe7612a2ced5299cc01202b1f6 3rdparty/libprocess/src/tests/process_tests.cpp 6b3aa1bcf20466cdf8f8249988b8b06dec27e5cd Diff: https://reviews.apache.org/r/45319/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 44250: Added `Resources::createStrippedScalarQuantity()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44250/ --- (Updated March 2, 2016, 6:27 p.m.) Review request for mesos and Joris Van Remoortere. Changes --- Tweak unit tests. Repository: mesos Description --- This returns a new `Resources` object that omits dynamic reservation and persistent volume information. This is intended primarily for situations in which code wants to efficiently compute aggregate statistics about many `Resource` values for which reservation and persistent volume information is not relevant. Diffs (updated) - include/mesos/resources.hpp fe8a5745ea7d4943c47ac22c73db70488c6dfa9f include/mesos/v1/resources.hpp c27927e4f0d7f45e69fe3312b2423afb64c5c51e src/common/resources.cpp 4fa1e78606485d6657d3776e28b78a43cc6449d2 src/tests/resources_tests.cpp e7525a00957e903993f4dd4b73e05c86f84c5c29 src/v1/resources.cpp bca523159577994d5890f832e4f61101b5dbf3bc Diff: https://reviews.apache.org/r/44250/diff/ Testing --- make check Thanks, Neil Conway
Review Request 44280: Removed `FLAGS_v` assignment in test case.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44280/ --- Review request for mesos and Joris Van Remoortere. Repository: mesos Description --- Per discussion with James Peach, this doesn't serve a useful purpose. Diffs - src/tests/hierarchical_allocator_tests.cpp 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 Diff: https://reviews.apache.org/r/44280/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 44251: Improved allocator perf for labeled reservations and volumes.
> On March 2, 2016, 1:11 p.m., Guangya Liu wrote: > > src/master/allocator/mesos/hierarchical.cpp, line 1421 > > <https://reviews.apache.org/r/44251/diff/2/?file=1276336#file1276336line1421> > > > > One suggestion is that does it make sense to update all variables > > including `scalar` to `strippedScalar` which might be more accurate. Thanks -- I decided to go with `scalarQuantity`. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44251/#review121633 --- On March 2, 2016, 7:15 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44251/ > --- > > (Updated March 2, 2016, 7:15 p.m.) > > > Review request for mesos and Joris Van Remoortere. > > > Bugs: MESOS-4833 > https://issues.apache.org/jira/browse/MESOS-4833 > > > Repository: mesos > > > Description > --- > > When the cluster contains many resources that have either labeled > reservations or persistent volumes, allocator performance can decrease > substantially because such metadata prevents `Resource` objects from > being merged together inside the allocator. As a result, the allocator > must manipulate `Resources` vectors that consist of many small > individual `Resource` values; since many `Resources` operations take > linear-time in the number of `Resource` values they contain, this can > cause very significant slowdowns. > > As a short-term solution, this commit strips dynamic reservation and > persistent volume information from the `Resources` objects used > internally by the allocator, because they are not needed when > aggregating resource quantities together. > > A long-term solution for this problem will be addressed as work on > refactoring the allocator more generally. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > 24fa50f62dec10ed43089297473cc386d6ba2f78 > src/master/allocator/sorter/drf/sorter.hpp > 46b2a9caf13b028a3aee6c1590679f885be90fd6 > src/master/allocator/sorter/drf/sorter.cpp > 9e863dd0ca5e2f2f0d517cb833687b757bed2c52 > src/master/allocator/sorter/sorter.hpp > ba91a38e47065718af87c9b3b7c5b74d25a258df > > Diff: https://reviews.apache.org/r/44251/diff/ > > > Testing > --- > > make check > > Perf: > > ``` > [ RUN ] HierarchicalAllocator_BENCHMARK_Test.ResourceLabels > Using 2000 slaves and 200 frameworks > round 0 allocate took 2.458462secs to make 200 offers > round 1 allocate took 2.427941secs to make 200 offers > round 2 allocate took 2.460724secs to make 200 offers > round 3 allocate took 2.443408secs to make 200 offers > round 4 allocate took 2.464784secs to make 200 offers > round 5 allocate took 2.501429secs to make 200 offers > round 6 allocate took 2.468777secs to make 200 offers > round 7 allocate took 2.482268secs to make 200 offers > round 8 allocate took 2.479014secs to make 200 offers > round 9 allocate took 2.529951secs to make 200 offers > round 10 allocate took 2.460059secs to make 200 offers > ``` > > Performance of `DeclineOffers` without labels is about ~2.1 seconds. > > > Thanks, > > Neil Conway > >
Re: Review Request 44251: Improved allocator perf for labeled reservations and volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44251/ --- (Updated March 2, 2016, 7:17 p.m.) Review request for mesos and Joris Van Remoortere. Changes --- Tweak variable name. Bugs: MESOS-4833 https://issues.apache.org/jira/browse/MESOS-4833 Repository: mesos Description --- When the cluster contains many resources that have either labeled reservations or persistent volumes, allocator performance can decrease substantially because such metadata prevents `Resource` objects from being merged together inside the allocator. As a result, the allocator must manipulate `Resources` vectors that consist of many small individual `Resource` values; since many `Resources` operations take linear-time in the number of `Resource` values they contain, this can cause very significant slowdowns. As a short-term solution, this commit strips dynamic reservation and persistent volume information from the `Resources` objects used internally by the allocator, because they are not needed when aggregating resource quantities together. A long-term solution for this problem will be addressed as work on refactoring the allocator more generally. Diffs (updated) - src/master/allocator/mesos/hierarchical.cpp 24fa50f62dec10ed43089297473cc386d6ba2f78 src/master/allocator/sorter/drf/sorter.hpp 46b2a9caf13b028a3aee6c1590679f885be90fd6 src/master/allocator/sorter/drf/sorter.cpp 9e863dd0ca5e2f2f0d517cb833687b757bed2c52 src/master/allocator/sorter/sorter.hpp ba91a38e47065718af87c9b3b7c5b74d25a258df Diff: https://reviews.apache.org/r/44251/diff/ Testing --- make check Perf: ``` [ RUN ] HierarchicalAllocator_BENCHMARK_Test.ResourceLabels Using 2000 slaves and 200 frameworks round 0 allocate took 2.458462secs to make 200 offers round 1 allocate took 2.427941secs to make 200 offers round 2 allocate took 2.460724secs to make 200 offers round 3 allocate took 2.443408secs to make 200 offers round 4 allocate took 2.464784secs to make 200 offers round 5 allocate took 2.501429secs to make 200 offers round 6 allocate took 2.468777secs to make 200 offers round 7 allocate took 2.482268secs to make 200 offers round 8 allocate took 2.479014secs to make 200 offers round 9 allocate took 2.529951secs to make 200 offers round 10 allocate took 2.460059secs to make 200 offers ``` Performance of `DeclineOffers` without labels is about ~2.1 seconds. Thanks, Neil Conway
Re: Review Request 44251: Improved allocator perf for labeled reservations and volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44251/ --- (Updated March 2, 2016, 7:15 p.m.) Review request for mesos and Joris Van Remoortere. Changes --- Tweak variable name per Guangya. Bugs: MESOS-4833 https://issues.apache.org/jira/browse/MESOS-4833 Repository: mesos Description --- When the cluster contains many resources that have either labeled reservations or persistent volumes, allocator performance can decrease substantially because such metadata prevents `Resource` objects from being merged together inside the allocator. As a result, the allocator must manipulate `Resources` vectors that consist of many small individual `Resource` values; since many `Resources` operations take linear-time in the number of `Resource` values they contain, this can cause very significant slowdowns. As a short-term solution, this commit strips dynamic reservation and persistent volume information from the `Resources` objects used internally by the allocator, because they are not needed when aggregating resource quantities together. A long-term solution for this problem will be addressed as work on refactoring the allocator more generally. Diffs (updated) - src/master/allocator/mesos/hierarchical.cpp 24fa50f62dec10ed43089297473cc386d6ba2f78 src/master/allocator/sorter/drf/sorter.hpp 46b2a9caf13b028a3aee6c1590679f885be90fd6 src/master/allocator/sorter/drf/sorter.cpp 9e863dd0ca5e2f2f0d517cb833687b757bed2c52 src/master/allocator/sorter/sorter.hpp ba91a38e47065718af87c9b3b7c5b74d25a258df Diff: https://reviews.apache.org/r/44251/diff/ Testing --- make check Perf: ``` [ RUN ] HierarchicalAllocator_BENCHMARK_Test.ResourceLabels Using 2000 slaves and 200 frameworks round 0 allocate took 2.458462secs to make 200 offers round 1 allocate took 2.427941secs to make 200 offers round 2 allocate took 2.460724secs to make 200 offers round 3 allocate took 2.443408secs to make 200 offers round 4 allocate took 2.464784secs to make 200 offers round 5 allocate took 2.501429secs to make 200 offers round 6 allocate took 2.468777secs to make 200 offers round 7 allocate took 2.482268secs to make 200 offers round 8 allocate took 2.479014secs to make 200 offers round 9 allocate took 2.529951secs to make 200 offers round 10 allocate took 2.460059secs to make 200 offers ``` Performance of `DeclineOffers` without labels is about ~2.1 seconds. Thanks, Neil Conway
Review Request 44371: Added CHANGELOG description for reservation labels.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44371/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- Added CHANGELOG description for reservation labels. Diffs - CHANGELOG 4530336b0b5480dc1935cf5fb30a9dd0f003b77c Diff: https://reviews.apache.org/r/44371/diff/ Testing --- Thanks, Neil Conway
Re: Review Request 44252: Update glog-0.3.3.patch to support PowerPC LE platform.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44252/#review121983 --- Is this patch in upstream glog? - Neil Conway On March 4, 2016, 1:39 a.m., Zhiwei Chen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44252/ > --- > > (Updated March 4, 2016, 1:39 a.m.) > > > Review request for mesos, Ben Mahler, Alex Clemmer, Kapil Arya, and Neil > Conway. > > > Bugs: MESOS-4312 > https://issues.apache.org/jira/browse/MESOS-4312 > > > Repository: mesos > > > Description > --- > > Update glog-0.3.3.patch to support PowerPC LE platform. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/glog-0.3.3.patch > 76b8c0fe3b4615371e265bab713d62c896b7c3d6 > > Diff: https://reviews.apache.org/r/44252/diff/ > > > Testing > --- > > > Thanks, > > Zhiwei Chen > >
Re: Review Request 44252: Update glog-0.3.3.patch to support PowerPC LE platform.
> On March 4, 2016, 1:40 a.m., Neil Conway wrote: > > Is this patch in upstream glog? > > Zhiwei Chen wrote: > No, this is PowerPC LE porting patch, and this patch can also work on > other platforms. > > As you can see the diff of this patch, it will not break other platforms. I mean, have you sent this patch to upstream glog? I would like to get us out of the business of patching glog at some point in the future. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44252/#review121983 --- On March 4, 2016, 1:39 a.m., Zhiwei Chen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44252/ > --- > > (Updated March 4, 2016, 1:39 a.m.) > > > Review request for mesos, Ben Mahler, Alex Clemmer, Kapil Arya, and Neil > Conway. > > > Bugs: MESOS-4312 > https://issues.apache.org/jira/browse/MESOS-4312 > > > Repository: mesos > > > Description > --- > > Update glog-0.3.3.patch to support PowerPC LE platform. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/glog-0.3.3.patch > 76b8c0fe3b4615371e265bab713d62c896b7c3d6 > > Diff: https://reviews.apache.org/r/44252/diff/ > > > Testing > --- > > > Thanks, > > Zhiwei Chen > >
Re: Review Request 44371: Added CHANGELOG description for reservation labels.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44371/ --- (Updated March 4, 2016, 12:55 a.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- Added CHANGELOG description for reservation labels. Diffs (updated) - CHANGELOG 4530336b0b5480dc1935cf5fb30a9dd0f003b77c Diff: https://reviews.apache.org/r/44371/diff/ Testing --- Thanks, Neil Conway
Review Request 44407: Fixed a typo in a log message in an example framework.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44407/ --- Review request for mesos and Joris Van Remoortere. Repository: mesos Description --- Fixed a typo in a log message in an example framework. Diffs - src/examples/long_lived_framework.cpp c4c3aa68dc3e6e001f9a746ea5151b8ad958856f Diff: https://reviews.apache.org/r/44407/diff/ Testing --- Visual inspection of log message. Thanks, Neil Conway
Re: Review Request 43685: Refactored test helper code.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43685/ --- (Updated March 2, 2016, 2:20 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Rebase. Repository: mesos Description --- Refactored test helper code. Diffs (updated) - src/tests/mesos.hpp 5f29085d7a491d8f672728228b824634ed42aeca src/tests/resources_tests.cpp a545100522bf4b1f03e50656d461b3cda6b41e11 Diff: https://reviews.apache.org/r/43685/diff/ Testing --- make check Thanks, Neil Conway
Review Request 44250: Added `Resources::createStrippedScalarQuantity()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44250/ --- Review request for mesos and Joris Van Remoortere. Repository: mesos Description --- This returns a new `Resources` object that omits dynamic reservation and persistent volume information. This is intended primarily for situations in which code wants to efficiently compute aggregate statistics about many `Resource` values for which reservation and persistent volume information is not relevant. Diffs - include/mesos/resources.hpp fe8a5745ea7d4943c47ac22c73db70488c6dfa9f include/mesos/v1/resources.hpp c27927e4f0d7f45e69fe3312b2423afb64c5c51e src/common/resources.cpp 4fa1e78606485d6657d3776e28b78a43cc6449d2 src/tests/resources_tests.cpp a545100522bf4b1f03e50656d461b3cda6b41e11 src/v1/resources.cpp bca523159577994d5890f832e4f61101b5dbf3bc Diff: https://reviews.apache.org/r/44250/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 43686: Added allocator benchmark using labeled resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43686/ --- (Updated March 2, 2016, 2:21 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Changed benchmark to isolate allocator bottleneck Summary (updated) - Added allocator benchmark using labeled resources. Bugs: MESOS-4691 https://issues.apache.org/jira/browse/MESOS-4691 Repository: mesos Description (updated) --- This reveals that when the cluster contains many reservations with distinct labels, allocator performance slows down dramatically. A short-term fix for this problem will be introduced shortly. Diffs (updated) - src/tests/hierarchical_allocator_tests.cpp 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 Diff: https://reviews.apache.org/r/43686/diff/ Testing (updated) --- make check FYI, results on my laptop: _Original benchmark (unlabeled resources)_ [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers Using 2000 slaves and 200 frameworks round 0 allocate took 2.028175secs to make 200 offers round 1 allocate took 2.006791secs to make 200 offers round 2 allocate took 2.033723secs to make 200 offers round 3 allocate took 2.017508secs to make 200 offers round 4 allocate took 2.037235secs to make 200 offers round 5 allocate took 2.054095secs to make 200 offers round 6 allocate took 2.048884secs to make 200 offers round 7 allocate took 2.044252secs to make 200 offers round 8 allocate took 2.060256secs to make 200 offers round 9 allocate took 2.07121secs to make 200 offers round 10 allocate took 2.066261secs to make 200 offers round 11 allocate took 2.034805secs to make 200 offers round 12 allocate took 2.053705secs to make 200 offers round 13 allocate took 2.042106secs to make 200 offers round 14 allocate took 2.082704secs to make 200 offers Thanks, Neil Conway
Re: Review Request 43684: Cleaned up allocator benchmark code.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43684/ --- (Updated March 2, 2016, 2:20 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Rebase. Repository: mesos Description --- Cleaned up allocator benchmark code. Diffs (updated) - src/tests/hierarchical_allocator_tests.cpp 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 Diff: https://reviews.apache.org/r/43684/diff/ Testing --- make check Thanks, Neil Conway
Review Request 44251: Improved allocator perf for labeled reservations and volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44251/ --- Review request for mesos and Joris Van Remoortere. Bugs: MESOS-4833 https://issues.apache.org/jira/browse/MESOS-4833 Repository: mesos Description --- When the cluster contains many resources that have either labeled reservations or persistent volumes, allocator performance can decrease substantially because such metadata prevents `Resource` objects from being merged together inside the allocator. As a result, the allocator must manipulate `Resources` vectors that consist of many small individual `Resource` values; since many `Resources` operations take linear-time in the number of `Resource` values they contain, this can cause very significant slowdowns. As a short-term solution, this commit strips dynamic reservation and persistent volume information from the `Resources` objects used internally by the allocator, because they are not needed when aggregating resource quantities together. A long-term solution for this problem will be addressed as work on refactoring the allocator more generally. Diffs - src/master/allocator/mesos/hierarchical.cpp 24fa50f62dec10ed43089297473cc386d6ba2f78 src/master/allocator/sorter/drf/sorter.hpp 46b2a9caf13b028a3aee6c1590679f885be90fd6 src/master/allocator/sorter/drf/sorter.cpp 9e863dd0ca5e2f2f0d517cb833687b757bed2c52 src/master/allocator/sorter/sorter.hpp ba91a38e47065718af87c9b3b7c5b74d25a258df Diff: https://reviews.apache.org/r/44251/diff/ Testing --- make check Perf: ``` [ RUN ] HierarchicalAllocator_BENCHMARK_Test.ResourceLabels Using 2000 slaves and 200 frameworks round 0 allocate took 2.458462secs to make 200 offers round 1 allocate took 2.427941secs to make 200 offers round 2 allocate took 2.460724secs to make 200 offers round 3 allocate took 2.443408secs to make 200 offers round 4 allocate took 2.464784secs to make 200 offers round 5 allocate took 2.501429secs to make 200 offers round 6 allocate took 2.468777secs to make 200 offers round 7 allocate took 2.482268secs to make 200 offers round 8 allocate took 2.479014secs to make 200 offers round 9 allocate took 2.529951secs to make 200 offers round 10 allocate took 2.460059secs to make 200 offers ``` Performance of `DeclineOffers` without labels is about ~2.1 seconds. Thanks, Neil Conway
Re: Review Request 43686: Added allocator benchmark using labeled resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43686/ --- (Updated March 2, 2016, 6:20 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Address code review comments. Bugs: MESOS-4691 https://issues.apache.org/jira/browse/MESOS-4691 Repository: mesos Description --- This reveals that when the cluster contains many reservations with distinct labels, allocator performance slows down dramatically. A short-term fix for this problem will be introduced shortly. Diffs (updated) - src/tests/hierarchical_allocator_tests.cpp 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 Diff: https://reviews.apache.org/r/43686/diff/ Testing --- make check FYI, results on my laptop: _Original benchmark (unlabeled resources)_ [ RUN ] HierarchicalAllocator_BENCHMARK_Test.DeclineOffers Using 2000 slaves and 200 frameworks round 0 allocate took 2.028175secs to make 200 offers round 1 allocate took 2.006791secs to make 200 offers round 2 allocate took 2.033723secs to make 200 offers round 3 allocate took 2.017508secs to make 200 offers round 4 allocate took 2.037235secs to make 200 offers round 5 allocate took 2.054095secs to make 200 offers round 6 allocate took 2.048884secs to make 200 offers round 7 allocate took 2.044252secs to make 200 offers round 8 allocate took 2.060256secs to make 200 offers round 9 allocate took 2.07121secs to make 200 offers round 10 allocate took 2.066261secs to make 200 offers round 11 allocate took 2.034805secs to make 200 offers round 12 allocate took 2.053705secs to make 200 offers round 13 allocate took 2.042106secs to make 200 offers round 14 allocate took 2.082704secs to make 200 offers Thanks, Neil Conway
Re: Review Request 43684: Cleaned up allocator benchmark code.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43684/ --- (Updated March 2, 2016, 6:20 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Changes --- Address code review comments. Repository: mesos Description --- Cleaned up allocator benchmark code. Diffs (updated) - src/tests/hierarchical_allocator_tests.cpp 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 Diff: https://reviews.apache.org/r/43684/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 44251: Improved allocator perf for labeled reservations and volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44251/ --- (Updated March 2, 2016, 6:21 a.m.) Review request for mesos and Joris Van Remoortere. Changes --- Address code review comments. Bugs: MESOS-4833 https://issues.apache.org/jira/browse/MESOS-4833 Repository: mesos Description --- When the cluster contains many resources that have either labeled reservations or persistent volumes, allocator performance can decrease substantially because such metadata prevents `Resource` objects from being merged together inside the allocator. As a result, the allocator must manipulate `Resources` vectors that consist of many small individual `Resource` values; since many `Resources` operations take linear-time in the number of `Resource` values they contain, this can cause very significant slowdowns. As a short-term solution, this commit strips dynamic reservation and persistent volume information from the `Resources` objects used internally by the allocator, because they are not needed when aggregating resource quantities together. A long-term solution for this problem will be addressed as work on refactoring the allocator more generally. Diffs (updated) - src/master/allocator/mesos/hierarchical.cpp 24fa50f62dec10ed43089297473cc386d6ba2f78 src/master/allocator/sorter/drf/sorter.hpp 46b2a9caf13b028a3aee6c1590679f885be90fd6 src/master/allocator/sorter/drf/sorter.cpp 9e863dd0ca5e2f2f0d517cb833687b757bed2c52 src/master/allocator/sorter/sorter.hpp ba91a38e47065718af87c9b3b7c5b74d25a258df Diff: https://reviews.apache.org/r/44251/diff/ Testing --- make check Perf: ``` [ RUN ] HierarchicalAllocator_BENCHMARK_Test.ResourceLabels Using 2000 slaves and 200 frameworks round 0 allocate took 2.458462secs to make 200 offers round 1 allocate took 2.427941secs to make 200 offers round 2 allocate took 2.460724secs to make 200 offers round 3 allocate took 2.443408secs to make 200 offers round 4 allocate took 2.464784secs to make 200 offers round 5 allocate took 2.501429secs to make 200 offers round 6 allocate took 2.468777secs to make 200 offers round 7 allocate took 2.482268secs to make 200 offers round 8 allocate took 2.479014secs to make 200 offers round 9 allocate took 2.529951secs to make 200 offers round 10 allocate took 2.460059secs to make 200 offers ``` Performance of `DeclineOffers` without labels is about ~2.1 seconds. Thanks, Neil Conway
Review Request 44348: Updated CHANGELOG for floating point resource changes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44348/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- Updated CHANGELOG for floating point resource changes. Diffs - CHANGELOG e85669ba815aa25ded3be878be0b799c89e66f2c Diff: https://reviews.apache.org/r/44348/diff/ Testing --- Thanks, Neil Conway
Re: Review Request 44348: Updated CHANGELOG for floating point resource changes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44348/ --- (Updated March 3, 2016, 8:14 p.m.) Review request for mesos and Vinod Kone. Changes --- Rebase, reindent. Repository: mesos Description --- Updated CHANGELOG for floating point resource changes. Diffs (updated) - CHANGELOG 7e1354abc3bf35e214d6566ca68124693cb8b8e2 Diff: https://reviews.apache.org/r/44348/diff/ Testing --- Thanks, Neil Conway
Re: Review Request 44433: Added empty line for list in maintenance doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44433/#review122354 --- Ship it! Ship It! - Neil Conway On March 6, 2016, 3:52 p.m., Klaus Ma wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44433/ > --- > > (Updated March 6, 2016, 3:52 p.m.) > > > Review request for mesos and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Added empty line for list in maintenance doc. > > > Diffs > - > > docs/maintenance.md 365c920719dbd0c5e61efe1975547a2848647bce > > Diff: https://reviews.apache.org/r/44433/diff/ > > > Testing > --- > > Document update. > > > Thanks, > > Klaus Ma > >
Re: Review Request 44478: Revised slave recovery documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44478/ --- (Updated March 7, 2016, 10:54 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- Revised slave recovery documentation. Diffs - docs/home.md 821026a12f422385e347e0037d6527efa9ffa2e1 docs/slave-recovery.md 5c148e5a39121f9b5cb7b5a84429551996c1116d Diff: https://reviews.apache.org/r/44478/diff/ Testing (updated) --- Previewed using site-docker. Thanks, Neil Conway
Review Request 44478: Revised slave recovery documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44478/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- Revised slave recovery documentation. Diffs - docs/home.md 821026a12f422385e347e0037d6527efa9ffa2e1 docs/slave-recovery.md 5c148e5a39121f9b5cb7b5a84429551996c1116d Diff: https://reviews.apache.org/r/44478/diff/ Testing --- Thanks, Neil Conway
Review Request 44477: Fixed typo in slave's `--help` output.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44477/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- Fixed typo in slave's `--help` output. Diffs - src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 Diff: https://reviews.apache.org/r/44477/diff/ Testing --- Thanks, Neil Conway
Review Request 44479: Revised HA framework guide documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44479/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- Revised HA framework guide documentation. Diffs - docs/high-availability-framework-guide.md 111d7b9094a7f36fef8cc95232f87c134630c031 Diff: https://reviews.apache.org/r/44479/diff/ Testing --- Thanks, Neil Conway
Review Request 44476: Revised comments about `link` semantics in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44476/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- Revised comments about `link` semantics in libprocess. Diffs - 3rdparty/libprocess/include/process/process.hpp c9ef4e86a4735c1c0342793b6d5144d80fc853a9 Diff: https://reviews.apache.org/r/44476/diff/ Testing --- Thanks, Neil Conway
Re: Review Request 44474: Improve master tasks metrics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44474/#review122401 --- src/master/master.cpp (line 6461) <https://reviews.apache.org/r/44474/#comment184371> Seems like we use post-increment elsewhere in this RR? src/master/metrics.cpp (line 327) <https://reviews.apache.org/r/44474/#comment184372> Why not use an iterator / foreach loop over `tasks_stats` instead? - Neil Conway On March 7, 2016, 10:51 p.m., Cong Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44474/ > --- > > (Updated March 7, 2016, 10:51 p.m.) > > > Review request for mesos, Ian Downes and Vinod Kone. > > > Bugs: MESOS-4740 > https://issues.apache.org/jira/browse/MESOS-4740 > > > Repository: mesos > > > Description > --- > > Avoid iterate the list of slaves, instead just maintain some counters. > > > Diffs > - > > src/master/master.hpp ea26670e6c6c67314406fded510e8fdd46053dc8 > src/master/master.cpp 57ff4a39039f573b8586bc03f873f97826b97f6f > src/master/metrics.hpp 9d201fcce1c46a890c86a889ab31029f9a061561 > src/master/metrics.cpp 30c091198a8fdd6d6a957a351dc37d3dae7788e4 > > Diff: https://reviews.apache.org/r/44474/diff/ > > > Testing > --- > > Manual check > > > Thanks, > > Cong Wang > >
Re: Review Request 44476: Revised comments about `link` semantics in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44476/ --- (Updated March 8, 2016, 12:13 a.m.) Review request for mesos and Vinod Kone. Changes --- Addressed code review comment. Repository: mesos Description --- Revised comments about `link` semantics in libprocess. Diffs (updated) - 3rdparty/libprocess/include/process/process.hpp c9ef4e86a4735c1c0342793b6d5144d80fc853a9 Diff: https://reviews.apache.org/r/44476/diff/ Testing --- Thanks, Neil Conway
Re: Review Request 44479: Revised HA framework guide documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44479/ --- (Updated March 8, 2016, 12:14 a.m.) Review request for mesos and Vinod Kone. Changes --- Fixed review comment. Repository: mesos Description --- Revised HA framework guide documentation. Diffs (updated) - docs/high-availability-framework-guide.md 111d7b9094a7f36fef8cc95232f87c134630c031 Diff: https://reviews.apache.org/r/44479/diff/ Testing --- Thanks, Neil Conway
Re: Review Request 45039: Updated the comment for launching tasks and accepting offers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45039/#review126586 --- docs/app-framework-development-guide.md (line 184) <https://reviews.apache.org/r/45039/#comment189599> Here and below, I would personally rephrase this slightly. "Any remaining resources remaining (i.e., those that are not used by the launched tasks or their executors) will be considered unused. Note that this includes resources used by tasks that the framework attempted to launch but failed (with TASK_ERROR) due to a malformed task description." - Neil Conway On April 1, 2016, 2:14 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45039/ > --- > > (Updated April 1, 2016, 2:14 p.m.) > > > Review request for mesos, Ben Mahler and Neil Conway. > > > Repository: mesos > > > Description > --- > > If the task does not pass validation, > its resources are considered declined. > > > Diffs > - > > docs/app-framework-development-guide.md > 1d8bebde67f69fd414509b8861571137d3569b46 > include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 > src/java/src/org/apache/mesos/SchedulerDriver.java > bf866f5ebece2505eaa27bf39a1382cd1a2a069a > src/python/interface/src/mesos/interface/__init__.py > 232890daa6d222ae1c86906bbc484c8e635c4eb7 > > Diff: https://reviews.apache.org/r/45039/diff/ > > > Testing > --- > > None: not a functional change. > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 45319: Cleaned up various code in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45319/ --- (Updated April 1, 2016, 4:20 p.m.) Review request for mesos and Ben Mahler. Changes --- More cleanup. Repository: mesos Description --- Cleaned up various code in libprocess. Diffs (updated) - 3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 3rdparty/libprocess/src/tests/benchmarks.cpp e8bbc55f0f9aeabe7612a2ced5299cc01202b1f6 3rdparty/libprocess/src/tests/process_tests.cpp 6b3aa1bcf20466cdf8f8249988b8b06dec27e5cd Diff: https://reviews.apache.org/r/45319/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 45318: Improved libprocess comments.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45318/ --- (Updated April 1, 2016, 4:20 p.m.) Review request for mesos and Ben Mahler. Changes --- More comment improvements. Summary (updated) - Improved libprocess comments. Repository: mesos Description (updated) --- Improved libprocess comments. Diffs (updated) - 3rdparty/libprocess/include/process/socket.hpp 4cb49680d1304899a4ee675ac07379e51d9c55b1 3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 Diff: https://reviews.apache.org/r/45318/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 42861: Ensure two Mount Disk resources do not have the same root path.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42861/#review125802 --- It would be good to have a unit test for the end-to-end behavior (i.e., we have a test for `path::overlapping` in the other RR, but we should also have a test for trying to create two volumes with overlapping paths). src/slave/slave.cpp (line 504) <https://reviews.apache.org/r/42861/#comment188683> Comment should be updated if we're going to do this even when you're not "verifying against the mount table entries". src/slave/slave.cpp (line 552) <https://reviews.apache.org/r/42861/#comment188681> Error message shouldn't end in punctuation. - Neil Conway On March 29, 2016, 2:30 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42861/ > --- > > (Updated March 29, 2016, 2:30 a.m.) > > > Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and > Neil Conway. > > > Bugs: MESOS-4521 > https://issues.apache.org/jira/browse/MESOS-4521 > > > Repository: mesos > > > Description > --- > > Ensure two Mount Disk resources do not have the same root path. > > > Diffs > - > > src/slave/slave.cpp f383605a52f31d7b805ad6153adc409dcb40f83a > > Diff: https://reviews.apache.org/r/42861/diff/ > > > Testing > --- > > > Thanks, > > haosdent huang > >
Re: Review Request 42860: Add paths::overlapping to check whether paths are overlapping.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42860/#review125801
---
Made a few quick comments. I didn't look at the `CheckOverlapTree` logic itself
yet though.
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 33)
<https://reviews.apache.org/r/42860/#comment188676>
Semicolon seems unnecessary.
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 53)
<https://reviews.apache.org/r/42860/#comment188677>
Semicolon seems unnecessary.
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 126)
<https://reviews.apache.org/r/42860/#comment188679>
Can we clarify what "overlapping" means here?
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 127)
<https://reviews.apache.org/r/42860/#comment188680>
What does "flat" mean, here? Also, is it worth adding a `CHECK` that all
paths are absolute?
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 128)
<https://reviews.apache.org/r/42860/#comment188678>
Should probably be `const std::vector&`.
- Neil Conway
On March 29, 2016, 2:30 a.m., haosdent huang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42860/
> ---
>
> (Updated March 29, 2016, 2:30 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and
> Neil Conway.
>
>
> Bugs: MESOS-4521
> https://issues.apache.org/jira/browse/MESOS-4521
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Add paths::overlapping to check whether paths are overlapping.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
> ef538045a8b7a1e3d8962c869317d86a85e0259f
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp
> 6dff5e76e0e15098c5a262adc50bfcb65f933697
>
> Diff: https://reviews.apache.org/r/42860/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 45489: Replaced reinterpret_cast with dynamic_cast in libprocess.
> On March 30, 2016, 3:03 p.m., Benjamin Bannier wrote: > > 3rdparty/libprocess/src/process.cpp, line 1531 > > <https://reviews.apache.org/r/45489/diff/1/?file=1319590#file1319590line1531> > > > > Here and below: Either explicitly assert that the `dynamic_cast` does > > not return `NULL`, or use `static_cast`. > > > > Since `Encoder` seems to implement its own idea of polymorphism it > > seems `static_cast` might fit better. Hmmm -- I thought `static_cast` was only safe for downcasts when the parent class is non-virtual? - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45489/#review126115 ------- On March 30, 2016, 2:49 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45489/ > --- > > (Updated March 30, 2016, 2:49 p.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos > > > Description > --- > > Casting from a parent class to a child class should not > be done with reinterpret_cast. > > > Diffs > - > > 3rdparty/libprocess/src/process.cpp > feaffa4334422ec3964f8d079f570061eaf390d2 > > Diff: https://reviews.apache.org/r/45489/diff/ > > > Testing > --- > > make check > > > Thanks, > > Neil Conway > >
Re: Review Request 45318: Added comments to some libprocess functions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45318/ --- (Updated March 30, 2016, 2:48 p.m.) Review request for mesos and Ben Mahler. Changes --- Added another comment. Summary (updated) - Added comments to some libprocess functions. Repository: mesos Description (updated) --- Added comments to some libprocess functions. Diffs (updated) - 3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 Diff: https://reviews.apache.org/r/45318/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 45319: Cleaned up various code in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45319/ --- (Updated March 30, 2016, 2:49 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Cleaned up various code in libprocess. Diffs (updated) - 3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 3rdparty/libprocess/src/tests/benchmarks.cpp e8bbc55f0f9aeabe7612a2ced5299cc01202b1f6 3rdparty/libprocess/src/tests/process_tests.cpp 6b3aa1bcf20466cdf8f8249988b8b06dec27e5cd Diff: https://reviews.apache.org/r/45319/diff/ Testing --- make check Thanks, Neil Conway
Review Request 45488: Removed an unnecessary `memset` from libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45488/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- Zero'ing the input buffer before receiving data is unnecessary. Moreover, keeping the `memset` around is confusing, because it makes the API contract of Socket.recv() less clear. Diffs - 3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 Diff: https://reviews.apache.org/r/45488/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 45466: Removed the redundant `NULL` check when deleting `Credential`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45466/#review126111 --- Ship it! Ship It! - Neil Conway On March 30, 2016, 2:23 a.m., Anand Mazumdar wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45466/ > --- > > (Updated March 30, 2016, 2:23 a.m.) > > > Review request for mesos, Klaus Ma, Neil Conway, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > See summary. Also moved all `delete` statements together in the order they > were declared in `.hpp` file. > > > Diffs > - > > src/sched/sched.cpp 701e024ac57fc6e2f96d7d83e65cb68252d41654 > > Diff: https://reviews.apache.org/r/45466/diff/ > > > Testing > --- > > make check > > > Thanks, > > Anand Mazumdar > >
Re: Review Request 45489: Replaced reinterpret_cast with static_cast in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45489/ --- (Updated March 30, 2016, 4:16 p.m.) Review request for mesos and Ben Mahler. Changes --- Use static_cast instead of dynamic_cast. Summary (updated) - Replaced reinterpret_cast with static_cast in libprocess. Repository: mesos Description (updated) --- Casting from a virtual base class to a child class should not be done with reinterpret_cast. We could use dynamic_cast as well, but that has a performance cost; in this case, we know exactly which child class the pointer points to, so the performance cost and additional safety offered by dynamic_cast is not necessary. Diffs (updated) - 3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 Diff: https://reviews.apache.org/r/45489/diff/ Testing (updated) --- make check with GCC 5.3 and recent apple-clang. Thanks, Neil Conway
Re: Review Request 45489: Replaced reinterpret_cast with static_cast in libprocess.
> On March 30, 2016, 3:03 p.m., Benjamin Bannier wrote: > > 3rdparty/libprocess/src/process.cpp, line 1531 > > <https://reviews.apache.org/r/45489/diff/1/?file=1319590#file1319590line1531> > > > > Here and below: Either explicitly assert that the `dynamic_cast` does > > not return `NULL`, or use `static_cast`. > > > > Since `Encoder` seems to implement its own idea of polymorphism it > > seems `static_cast` might fit better. > > Neil Conway wrote: > Hmmm -- I thought `static_cast` was only safe for downcasts when the > parent class is non-virtual? > > Benjamin Bannier wrote: > It is fine for most cases, but cannot be used e.g., in the presence of > virtual *inheritance* (which isn't used here). Double-checked with mpark -- sounds good. Thanks! - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45489/#review126115 ------- On March 30, 2016, 4:16 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45489/ > --- > > (Updated March 30, 2016, 4:16 p.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos > > > Description > --- > > Casting from a virtual base class to a child class should not be > done with reinterpret_cast. We could use dynamic_cast as well, but > that has a performance cost; in this case, we know exactly which > child class the pointer points to, so the performance cost and > additional safety offered by dynamic_cast is not necessary. > > > Diffs > - > > 3rdparty/libprocess/src/process.cpp > feaffa4334422ec3964f8d079f570061eaf390d2 > > Diff: https://reviews.apache.org/r/45489/diff/ > > > Testing > --- > > make check with GCC 5.3 and recent apple-clang. > > > Thanks, > > Neil Conway > >
Re: Review Request 45562: Edited `--work_dir` help strings and docs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45562/#review126442 --- docs/configuration.md (line 361) <https://reviews.apache.org/r/45562/#comment189430> What's an "automatic garbage collection service"? Can we spell out what bad stuff might happen if `/tmp` is used for `work_dir`? docs/configuration.md (line 1598) <https://reviews.apache.org/r/45562/#comment189431> Might be worth noting that the default value _is_ not consistent with this warning. - Neil Conway On March 31, 2016, 8:47 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45562/ > --- > > (Updated March 31, 2016, 8:47 p.m.) > > > Review request for mesos, Jie Yu and Neil Conway. > > > Bugs: MESOS-5064 > https://issues.apache.org/jira/browse/MESOS-5064 > > > Repository: mesos > > > Description > --- > > Some users have encountered difficulty when running the Mesos agent with the > `work_dir` located in a subdirectory of `/tmp`. This patch adds language to > the `work_dir` help strings and configuration docs advising users to avoid > the use of this location in production. > > > Diffs > - > > docs/configuration.md da42eaf7069a016fa7eaf929fc285e1fa1f144e9 > src/master/flags.cpp 06852c9de68cce5d40f294f6402f7677ee6183d3 > src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 > > Diff: https://reviews.apache.org/r/45562/diff/ > > > Testing > --- > > Viewed with the Mesos website container: > https://github.com/mesosphere/mesos-website-container > > > Thanks, > > Greg Mann > >
Re: Review Request 45122: Updated tests for deletion of persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45122/ --- (Updated March 28, 2016, 4:07 p.m.) Review request for mesos, Jie Yu and Joris Van Remoortere. Changes --- Rename test case. Bugs: MESOS-2408 https://issues.apache.org/jira/browse/MESOS-2408 Repository: mesos Description --- Updated tests for deletion of persistent volumes. Diffs (updated) - src/tests/persistent_volume_tests.cpp 26fff19daa8b175fdcc06fd9467224d5920a1967 Diff: https://reviews.apache.org/r/45122/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 45121: Implemented deletion for persistent volumes.
> On March 26, 2016, 12:37 a.m., Jie Yu wrote: > > src/slave/slave.cpp, line 2375 > > <https://reviews.apache.org/r/45121/diff/1/?file=1308847#file1308847line2375> > > > > I would suggest we don't use CHECK here. We can just LOG(ERROR) if the > > deletion fails. Given the TODO above, we could leak disk space anyway. > > Neil Conway wrote: > I used `CHECK_SOME` because we use the same error-handling strategy when > creating the persistent volume fails, or if the slave fails to checkpoint > resources to disk successfully. Are you sure we want to handle `rmdir` errors > differently? > > Jie Yu wrote: > We cannot proceed when creating persistent volume fails because the > subsequent 'mount' will fail. > > For the deletion case, if it fails, we just leak some data in the volume. > The subsequent operation will not fail. Since we'll be leaking data anyway > (the slave crash case), I suggest that we don't use CHECK_SOME here. True, although for MOUNT disks we might expose data from the previous volume to a newly-created volume on the same disk. Overall I agree that using `LOG(ERROR)` is probably best for now. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45121/#review125492 ------- On March 27, 2016, 6:08 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45121/ > --- > > (Updated March 27, 2016, 6:08 p.m.) > > > Review request for mesos, Jie Yu and Joris Van Remoortere. > > > Bugs: MESOS-2408 > https://issues.apache.org/jira/browse/MESOS-2408 > > > Repository: mesos > > > Description > --- > > Prior to this commit, destroying a persistent volume would remove > the Mesos-level metadata about the volume, but wouldn't destroy > any of the volume's filesystem content. We now remove the volume > from the slave's filesystem, essentially via "rm -r". > > > Diffs > - > > src/slave/slave.cpp f383605a52f31d7b805ad6153adc409dcb40f83a > > Diff: https://reviews.apache.org/r/45121/diff/ > > > Testing > --- > > make check > > > Thanks, > > Neil Conway > >
Re: Review Request 45122: Updated tests for deletion of persistent volumes.
> On March 26, 2016, 12:53 a.m., Jie Yu wrote: > > src/tests/persistent_volume_tests.cpp, line 243 > > <https://reviews.apache.org/r/45122/diff/1/?file=1308848#file1308848line243> > > > > Can we create a new test, instead of piggyback on this test? In the new > > test, you can test the 'creation' of persistent volume as well for PATH > > type disk. > > Neil Conway wrote: > I looked at creating a new test, although it seemed like it would just > duplicate all of the existing `SendingCheckpointResourcesMessage` test. For > now I haven't made a new test, although I can if you feel strongly about it. > > Re: testing the creation of PATH disks, can you clarify? AFAIK we're > testing that right now. > > Jie Yu wrote: > OK, maybe rename this test? like CreateAndDestroy? Okay, renamed to `CreateAndDestroyPersistentVolumes`. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45122/#review125499 ------- On March 27, 2016, 10:05 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45122/ > --- > > (Updated March 27, 2016, 10:05 p.m.) > > > Review request for mesos, Jie Yu and Joris Van Remoortere. > > > Bugs: MESOS-2408 > https://issues.apache.org/jira/browse/MESOS-2408 > > > Repository: mesos > > > Description > --- > > Updated tests for deletion of persistent volumes. > > > Diffs > - > > src/tests/persistent_volume_tests.cpp > 26fff19daa8b175fdcc06fd9467224d5920a1967 > > Diff: https://reviews.apache.org/r/45122/diff/ > > > Testing > --- > > make check > > > Thanks, > > Neil Conway > >
Re: Review Request 45122: Updated tests for deletion of persistent volumes.
> On March 26, 2016, 12:53 a.m., Jie Yu wrote: > > src/tests/persistent_volume_tests.cpp, line 810 > > <https://reviews.apache.org/r/45122/diff/1/?file=1308848#file1308848line810> > > > > What the purpose of adding another file here? > > Neil Conway wrote: > I wanted to check that if files are directly written into the directory > that corresponds to the persistent volume (not via the task itself), they are > still cleaned up when the volume is destroyed. > > Jie Yu wrote: > hum, I still don't understand. What's the purpose of this test? to > simulate what situation? The situation I wanted to test is that you have a directory `/foo` that is being used by a Mesos task as a persistent volume. Another process on the box writes a file `/foo/bar`, and then the Mesos task terminates and the volume is destroyed. The test checks that `/foo/bar` is removed, even though it wasn't created by the Mesos task. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45122/#review125499 ------- On March 27, 2016, 10:05 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45122/ > --- > > (Updated March 27, 2016, 10:05 p.m.) > > > Review request for mesos, Jie Yu and Joris Van Remoortere. > > > Bugs: MESOS-2408 > https://issues.apache.org/jira/browse/MESOS-2408 > > > Repository: mesos > > > Description > --- > > Updated tests for deletion of persistent volumes. > > > Diffs > - > > src/tests/persistent_volume_tests.cpp > 26fff19daa8b175fdcc06fd9467224d5920a1967 > > Diff: https://reviews.apache.org/r/45122/diff/ > > > Testing > --- > > make check > > > Thanks, > > Neil Conway > >
Re: Review Request 45423: Added note about preventing resource autodetection to documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45423/#review125874 --- Fix it, then Ship it! docs/multiple-disk.md (line 31) <https://reviews.apache.org/r/45423/#comment188768> suggested: "manually (via the `--resources` flag)" docs/multiple-disk.md (line 33) <https://reviews.apache.org/r/45423/#comment188769> suggested: "manually specify it using the format described below." - Neil Conway On March 29, 2016, 10:24 a.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45423/ > --- > > (Updated March 29, 2016, 10:24 a.m.) > > > Review request for mesos, Joris Van Remoortere and Neil Conway. > > > Repository: mesos > > > Description > --- > > When manually specifying disk resources as explained in the multiple-disk > documentation, Mesos stops autodetecting the Root disk. This can be > unexpected behavior for users. > > > Diffs > - > > docs/multiple-disk.md 5c15b64b8b7737159cfba21223a84bdfcac75a0b > > Diff: https://reviews.apache.org/r/45423/diff/ > > > Testing > --- > > Viewed as gist: https://gist.github.com/joerg84/ee8a6b5a92e71d6bf704 > > > Thanks, > > Joerg Schad > >
Re: Review Request 45122: Updated tests for deletion of persistent volumes.
> On March 26, 2016, 12:53 a.m., Jie Yu wrote: > > src/tests/persistent_volume_tests.cpp, line 810 > > <https://reviews.apache.org/r/45122/diff/1/?file=1308848#file1308848line810> > > > > What the purpose of adding another file here? > > Neil Conway wrote: > I wanted to check that if files are directly written into the directory > that corresponds to the persistent volume (not via the task itself), they are > still cleaned up when the volume is destroyed. > > Jie Yu wrote: > hum, I still don't understand. What's the purpose of this test? to > simulate what situation? > > Neil Conway wrote: > The situation I wanted to test is that you have a directory `/foo` that > is being used by a Mesos task as a persistent volume. Another process on the > box writes a file `/foo/bar`, and then the Mesos task terminates and the > volume is destroyed. The test checks that `/foo/bar` is removed, even though > it wasn't created by the Mesos task. > > Jie Yu wrote: > I understand what it is doing. I don't understand is: is this a > legitimate use case? You mean some out of band process is writing the > persistent volume? This does not fit well with the Mesos model, isn't it? I > would suggest we don't add those. Well, we're already testing whether an out-of-band process can read from the persistent volume (for `filePath1`), so it seemed natural to test writing to it as well -- happy to remove it if you'd prefer though. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45122/#review125499 --- On March 28, 2016, 4:07 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45122/ > --- > > (Updated March 28, 2016, 4:07 p.m.) > > > Review request for mesos, Jie Yu and Joris Van Remoortere. > > > Bugs: MESOS-2408 > https://issues.apache.org/jira/browse/MESOS-2408 > > > Repository: mesos > > > Description > --- > > Updated tests for deletion of persistent volumes. > > > Diffs > - > > src/tests/persistent_volume_tests.cpp > 26fff19daa8b175fdcc06fd9467224d5920a1967 > > Diff: https://reviews.apache.org/r/45122/diff/ > > > Testing > --- > > make check > > > Thanks, > > Neil Conway > >
Re: Review Request 45122: Updated tests for deletion of persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45122/ --- (Updated March 28, 2016, 6:20 p.m.) Review request for mesos, Jie Yu and Joris Van Remoortere. Changes --- Remove writing a file to persistent volume. Bugs: MESOS-2408 https://issues.apache.org/jira/browse/MESOS-2408 Repository: mesos Description --- Updated tests for deletion of persistent volumes. Diffs (updated) - src/tests/persistent_volume_tests.cpp 26fff19daa8b175fdcc06fd9467224d5920a1967 Diff: https://reviews.apache.org/r/45122/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 45122: Updated tests for deletion of persistent volumes.
> On March 26, 2016, 12:53 a.m., Jie Yu wrote: > > src/tests/persistent_volume_tests.cpp, line 810 > > <https://reviews.apache.org/r/45122/diff/1/?file=1308848#file1308848line810> > > > > What the purpose of adding another file here? > > Neil Conway wrote: > I wanted to check that if files are directly written into the directory > that corresponds to the persistent volume (not via the task itself), they are > still cleaned up when the volume is destroyed. > > Jie Yu wrote: > hum, I still don't understand. What's the purpose of this test? to > simulate what situation? > > Neil Conway wrote: > The situation I wanted to test is that you have a directory `/foo` that > is being used by a Mesos task as a persistent volume. Another process on the > box writes a file `/foo/bar`, and then the Mesos task terminates and the > volume is destroyed. The test checks that `/foo/bar` is removed, even though > it wasn't created by the Mesos task. > > Jie Yu wrote: > I understand what it is doing. I don't understand is: is this a > legitimate use case? You mean some out of band process is writing the > persistent volume? This does not fit well with the Mesos model, isn't it? I > would suggest we don't add those. > > Neil Conway wrote: > Well, we're already testing whether an out-of-band process can read from > the persistent volume (for `filePath1`), so it seemed natural to test writing > to it as well -- happy to remove it if you'd prefer though. > > Jie Yu wrote: > I think the 'read' is to make sure the file written by the task is > actually persisted in the persistent volume. Okay, removed. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45122/#review125499 --- On March 28, 2016, 6:20 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45122/ > --- > > (Updated March 28, 2016, 6:20 p.m.) > > > Review request for mesos, Jie Yu and Joris Van Remoortere. > > > Bugs: MESOS-2408 > https://issues.apache.org/jira/browse/MESOS-2408 > > > Repository: mesos > > > Description > --- > > Updated tests for deletion of persistent volumes. > > > Diffs > - > > src/tests/persistent_volume_tests.cpp > 26fff19daa8b175fdcc06fd9467224d5920a1967 > > Diff: https://reviews.apache.org/r/45122/diff/ > > > Testing > --- > > make check > > > Thanks, > > Neil Conway > >
Re: Review Request 45350: Add `--cgroups_subsystems` in agent flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45350/#review126858 --- src/slave/flags.cpp (line 335) <https://reviews.apache.org/r/45350/#comment189946> Remove the whitespace before "\n". - Neil Conway On April 4, 2016, 5:14 p.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45350/ > --- > > (Updated April 4, 2016, 5:14 p.m.) > > > Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, > Kevin Klues, and Neil Conway. > > > Bugs: MESOS-5040 > https://issues.apache.org/jira/browse/MESOS-5040 > > > Repository: mesos > > > Description > --- > > Add `--cgroups_subsystems` in agent flags. > > > Diffs > - > > docs/configuration.md da42eaf7069a016fa7eaf929fc285e1fa1f144e9 > src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 > src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 > > Diff: https://reviews.apache.org/r/45350/diff/ > > > Testing > --- > > > Thanks, > > haosdent huang > >
Review Request 46146: Fixed libprocess tests to use smart pointers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46146/ --- Review request for mesos, Benjamin Bannier and Joris Van Remoortere. Repository: mesos Description --- This ensures that dynamically allocated memory is released when a test aborts prematurely due to an EXPECT_XXX failure. Diffs - 3rdparty/libprocess/src/tests/decoder_tests.cpp bd990c5eb77e47d7f617199bcc89e9432af7cc51 3rdparty/libprocess/src/tests/process_tests.cpp 274a76fa61a5cd4b1324be124f73979d4b980660 3rdparty/libprocess/src/tests/sequence_tests.cpp 566393fbf3f19644b86843f07194d1de36a2015e Diff: https://reviews.apache.org/r/46146/diff/ Testing --- make check on OSX and Linux. Thanks, Neil Conway
Re: Review Request 46094: Fixed memory leaks in Encoder/Decoder tests in libprocess.
> On April 12, 2016, 2:16 p.m., Benjamin Bannier wrote: > > 3rdparty/libprocess/src/tests/decoder_tests.cpp, line 202 > > <https://reviews.apache.org/r/46094/diff/1/?file=1341387#file1341387line202> > > > > Here and everywhere below: This still leaks if any of above `EXPECT_*` > > fail; in that case we continue running tests. > > > > Since we `ASSERT` that we only got a single response we might as well > > do just > > > > Owned reponse = response[0]; > > > > If the goal of this nice effort is to be able to run tests under leak > > checkers (and potentially correlate failures to memory problems), I'd > > really appreciate using some smart pointer for automatic cleanup instead. Good point! Adjusted this commit to use `Owned` instead, and submitted an additional RR that makes use of `Owned` in a few other places in the libprocess tests. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46094/#review128404 ------- On April 12, 2016, 2:02 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46094/ > --- > > (Updated April 12, 2016, 2:02 p.m.) > > > Review request for mesos, Joris Van Remoortere and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Fixed memory leaks in Encoder/Decoder tests in libprocess. > > > Diffs > - > > 3rdparty/libprocess/src/tests/decoder_tests.cpp > bd990c5eb77e47d7f617199bcc89e9432af7cc51 > 3rdparty/libprocess/src/tests/encoder_tests.cpp > 61ec8d8722245179a929e954e6338606973b819b > > Diff: https://reviews.apache.org/r/46094/diff/ > > > Testing > --- > > make check > > Verified that the number of leaked allocations decreases (from ~129 to ~92) > with this change. Obviously there are more leaks to investigate but at first > glance they seem a bit more subtle. > > > Thanks, > > Neil Conway > >
Re: Review Request 46094: Fixed memory leaks in Encoder/Decoder tests in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46094/ --- (Updated April 13, 2016, 2:23 p.m.) Review request for mesos, Joris Van Remoortere and Joseph Wu. Changes --- Use smart pointers. Repository: mesos Description --- Fixed memory leaks in Encoder/Decoder tests in libprocess. Diffs (updated) - 3rdparty/libprocess/src/tests/decoder_tests.cpp bd990c5eb77e47d7f617199bcc89e9432af7cc51 3rdparty/libprocess/src/tests/encoder_tests.cpp 61ec8d8722245179a929e954e6338606973b819b Diff: https://reviews.apache.org/r/46094/diff/ Testing --- make check Verified that the number of leaked allocations decreases (from ~129 to ~92) with this change. Obviously there are more leaks to investigate but at first glance they seem a bit more subtle. Thanks, Neil Conway
Re: Review Request 46593: Added test for containerizer destroy while provisioning race.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46593/#review130418 --- Fix it, then Ship it! src/tests/containerizer/mesos_containerizer_tests.cpp (line 958) <https://reviews.apache.org/r/46593/#comment194160> Extra set of parentheses here doesn't seem useful. (Can you fix this where it occurs elsewhere in the test file?) - Neil Conway On April 22, 2016, 11:48 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46593/ > --- > > (Updated April 22, 2016, 11:48 p.m.) > > > Review request for mesos, Artem Harutyunyan, Jie Yu, Neil Conway, and Timothy > Chen. > > > Repository: mesos > > > Description > --- > > Added test for containerizer destroy while provisioning race. > > > Diffs > - > > src/tests/containerizer/mesos_containerizer_tests.cpp > 09742ff21513dc2570684d384b257868dd57a9ce > > Diff: https://reviews.apache.org/r/46593/diff/ > > > Testing > --- > > make check > > Verified that the test is 90% chance to be segfault if we reverted the fix. > > > Thanks, > > Gilbert Song > >
Re: Review Request 40266: Libprocess Reinit: Cleanup SocketManager alongside ProcessManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40266/#review130501 --- 3rdparty/libprocess/src/process.cpp (line 1356) <https://reviews.apache.org/r/40266/#comment194255> "each socket" 3rdparty/libprocess/src/process.cpp (line 1357) <https://reviews.apache.org/r/40266/#comment194256> "interdependency" => "a dependency" - Neil Conway On April 14, 2016, 8:28 p.m., Joseph Wu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40266/ > --- > > (Updated April 14, 2016, 8:28 p.m.) > > > Review request for mesos, Artem Harutyunyan and Joris Van Remoortere. > > > Bugs: MESOS-3910 > https://issues.apache.org/jira/browse/MESOS-3910 > > > Repository: mesos > > > Description > --- > > The `SocketManager` and `ProcessManager` are highly inter-dependent, > which requires some untangling in `process::finalize`. > > * Logic originally found in `~ProcessManager` has been split into > `ProcessManager::finalize` due to what happens during cleanup. > * The future from `__s__->accept()` must be explicitly discarded as > libevent does not detect a locally closed socket. > > > Diffs > - > > 3rdparty/libprocess/src/process.cpp > afeddec20495bb9621c3e26b0d425c9419654739 > > Diff: https://reviews.apache.org/r/40266/diff/ > > > Testing > --- > > `make check` (libev) > `make check` (--enable-libevent --enable-ssl) > > > Thanks, > > Joseph Wu > >
Re: Review Request 40512: Libprocess Reinit: Add a test-only method to reinitialize libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40512/#review130500 --- 3rdparty/libprocess/src/process.cpp (line 475) <https://reviews.apache.org/r/40512/#comment194253> Typo. - Neil Conway On April 14, 2016, 8:28 p.m., Joseph Wu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40512/ > --- > > (Updated April 14, 2016, 8:28 p.m.) > > > Review request for mesos, Artem Harutyunyan and Joris Van Remoortere. > > > Bugs: MESOS-3820 > https://issues.apache.org/jira/browse/MESOS-3820 > > > Repository: mesos > > > Description > --- > > This builds upon earlier changes to complete `process::finalize`. > Tests which need to reconfigure libprocess, such as some SSL-related > tests, can use `process::reinitialize` to do so. > > This method may also be useful for providing additional isolation > between tests, such as cleaning up all processes after each test case. > > > Diffs > - > > 3rdparty/libprocess/src/openssl.cpp > 523b9740a45bdae3fa9a1a1423108e6815fbf872 > 3rdparty/libprocess/src/process.cpp > afeddec20495bb9621c3e26b0d425c9419654739 > > Diff: https://reviews.apache.org/r/40512/diff/ > > > Testing > --- > > Defined `process::reinitialize` in several tests, such as `HTTPTest`, > `MetricsTest`, and `ReapTest` can called `process::reinitialize` before and > after tests. Confirmed that this did not have side effects on other tests > (See https://reviews.apache.org/r/40453/ ). > > > Thanks, > > Joseph Wu > >
Re: Review Request 46471: Updated descriptions for master endpoints which may return redirect.
> On April 22, 2016, 3:11 p.m., Neil Conway wrote: > > > > haosdent huang wrote: > Hi, @neilc. Thank you very much for your detail comments! I saw you use > `is xxed` in some comments while use `was xxed` in others. Should I change > all of them to `was xxed` to keep consistent? Good point -- I don't have a strong preference on using "is" vs. "was", but it would be good to pick one and use it consistently. I suppose "was" is slightly more accurate? - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46471/#review130099 --- On April 22, 2016, 3:48 p.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46471/ > --- > > (Updated April 22, 2016, 3:48 p.m.) > > > Review request for mesos, Adam B, Anand Mazumdar, Cody Maloney, Ian Downes, > Neil Conway, and Vinod Kone. > > > Bugs: MESOS-1865 > https://issues.apache.org/jira/browse/MESOS-1865 > > > Repository: mesos > > > Description > --- > > Update master endpoints descriptions about `TEMPORARY_REDIRECT` and > `SERVICE_UNAVAILABLE` when current master is not the leader. > > > Diffs > - > > src/master/http.cpp de06985cafc93022fffc0d13812a119ae43b7e57 > > Diff: https://reviews.apache.org/r/46471/diff/ > > > Testing > --- > > > Thanks, > > haosdent huang > >
Re: Review Request 46471: Updated descriptions for master endpoints which may return redirect.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46471/#review130112
---
Fix it, then Ship it!
Ship It!
src/master/http.cpp (line 2339)
<https://reviews.apache.org/r/46471/#comment193780>
"was successful"
src/master/http.cpp (line 2455)
<https://reviews.apache.org/r/46471/#comment193781>
"was successful"
- Neil Conway
On April 22, 2016, 4:11 p.m., haosdent huang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46471/
> ---
>
> (Updated April 22, 2016, 4:11 p.m.)
>
>
> Review request for mesos, Adam B, Anand Mazumdar, Cody Maloney, Ian Downes,
> Neil Conway, and Vinod Kone.
>
>
> Bugs: MESOS-1865
> https://issues.apache.org/jira/browse/MESOS-1865
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Update master endpoints descriptions about `TEMPORARY_REDIRECT` and
> `SERVICE_UNAVAILABLE` when current master is not the leader.
>
>
> Diffs
> -
>
> src/master/http.cpp de06985cafc93022fffc0d13812a119ae43b7e57
>
> Diff: https://reviews.apache.org/r/46471/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 46473: Updated `high-availability` and `operational-guide` docs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46473/#review130113 --- Ship it! Ship It! - Neil Conway On April 22, 2016, 4:10 p.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46473/ > --- > > (Updated April 22, 2016, 4:10 p.m.) > > > Review request for mesos, Adam B, Anand Mazumdar, Cody Maloney, Ian Downes, > Neil Conway, and Vinod Kone. > > > Bugs: MESOS-1865 > https://issues.apache.org/jira/browse/MESOS-1865 > > > Repository: mesos > > > Description > --- > > In this changes, we update the `high-availability` and > `operational-guide` documents about master http endpoints redirction > when current master is not the leader. > > > Diffs > - > > docs/high-availability.md b0e744e62081b56e2a24ef5f7304f424424fa3cc > docs/operational-guide.md 5ae7ede3f500380a78364d5c3da2c4cea75e04c5 > > Diff: https://reviews.apache.org/r/46473/diff/ > > > Testing > --- > > > Thanks, > > haosdent huang > >
Re: Review Request 46471: Updated descriptions for master endpoints which may return redirect.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46471/#review130099
---
src/master/http.cpp (line 335)
<https://reviews.apache.org/r/46471/#comment193747>
Can we use the same phrasing ("if the leading master cannot be found") here
as we do below?
src/master/http.cpp (line 690)
<https://reviews.apache.org/r/46471/#comment193748>
Lowercase "r" in "redirect", here and below.
src/master/http.cpp (line 795)
<https://reviews.apache.org/r/46471/#comment193750>
"when the frameworks info is queried successfully."
src/master/http.cpp (line 1175)
<https://reviews.apache.org/r/46471/#comment193751>
"Returns 200 OK when the request is processed successfully."
src/master/http.cpp (line 1259)
<https://reviews.apache.org/r/46471/#comment193752>
"Returns 200 OK when the quota has been changed successfully."
src/master/http.cpp (line 1307)
<https://reviews.apache.org/r/46471/#comment193753>
"Returns 200 OK when the weight update was successful."
src/master/http.cpp (line 1350)
<https://reviews.apache.org/r/46471/#comment193754>
"Returns 200 OK when the state of the master was queried successfully."
src/master/http.cpp (line 1706)
<https://reviews.apache.org/r/46471/#comment193755>
"Returns 200 OK when a summary of the master's state was queried
successfully."
src/master/http.cpp (line 1836)
<https://reviews.apache.org/r/46471/#comment193756>
"Returns 200 OK when information about roles was queried successfully."
src/master/http.cpp (line 1957)
<https://reviews.apache.org/r/46471/#comment193758>
Not years, but "Returns 200 OK if the framework was torn down successfully."
src/master/http.cpp (line 2054)
<https://reviews.apache.org/r/46471/#comment193759>
"Returns 200 OK when task information was queried successfully."
src/master/http.cpp (line 2188)
<https://reviews.apache.org/r/46471/#comment193749>
"when maintenance successfully" is ungrammatical. "when the requested
maintenance operation was performed successfully."
src/master/http.cpp (line 2337)
<https://reviews.apache.org/r/46471/#comment193760>
"Returns 200 OK when the operation is successful."
src/master/http.cpp (line 2453)
<https://reviews.apache.org/r/46471/#comment193761>
"Returns 200 OK when the operation is successful."
src/master/http.cpp (line 2568)
<https://reviews.apache.org/r/46471/#comment193762>
"Returns 200 OK when the maintenance status was queried successfully."
- Neil Conway
On April 22, 2016, 7:18 a.m., haosdent huang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46471/
> ---
>
> (Updated April 22, 2016, 7:18 a.m.)
>
>
> Review request for mesos, Adam B, Anand Mazumdar, Cody Maloney, Ian Downes,
> Neil Conway, and Vinod Kone.
>
>
> Bugs: MESOS-1865
> https://issues.apache.org/jira/browse/MESOS-1865
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Update master endpoints descriptions about `TEMPORARY_REDIRECT` and
> `SERVICE_UNAVAILABLE` when current master is not the leader.
>
>
> Diffs
> -
>
> src/master/http.cpp de06985cafc93022fffc0d13812a119ae43b7e57
>
> Diff: https://reviews.apache.org/r/46471/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 46391: Clarified several agent log messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46391/ --- (Updated April 22, 2016, 3:28 p.m.) Review request for mesos, Anand Mazumdar and Vinod Kone. Changes --- Address review comment. Repository: mesos Description --- When diagnosing problems, it can be useful to distinguish between executors that are "terminating" vs. "terminated". Diffs (updated) - src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526 Diff: https://reviews.apache.org/r/46391/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 46471: Updated descriptions for master endpoints which may return redirect.
> On April 22, 2016, 3:11 p.m., Neil Conway wrote: > > src/master/http.cpp, line 1307 > > <https://reviews.apache.org/r/46471/diff/3/?file=1356457#file1356457line1307> > > > > "Returns 200 OK when the weight update was successful." > > haosdent huang wrote: > Should it be `the weights update` here? Yes, thanks! - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46471/#review130099 --- On April 22, 2016, 7:18 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46471/ > --- > > (Updated April 22, 2016, 7:18 a.m.) > > > Review request for mesos, Adam B, Anand Mazumdar, Cody Maloney, Ian Downes, > Neil Conway, and Vinod Kone. > > > Bugs: MESOS-1865 > https://issues.apache.org/jira/browse/MESOS-1865 > > > Repository: mesos > > > Description > --- > > Update master endpoints descriptions about `TEMPORARY_REDIRECT` and > `SERVICE_UNAVAILABLE` when current master is not the leader. > > > Diffs > - > > src/master/http.cpp de06985cafc93022fffc0d13812a119ae43b7e57 > > Diff: https://reviews.apache.org/r/46471/diff/ > > > Testing > --- > > > Thanks, > > haosdent huang > >
Re: Review Request 46473: Updated `high-availability` and `operational-guide` docs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46473/#review130101 --- docs/high-availability.md (line 29) <https://reviews.apache.org/r/46473/#comment193763> Can we link to the endpoint docs for `/redirect` here? i.e., [/redirect](endpoint/master/redirect.md) docs/high-availability.md (line 30) <https://reviews.apache.org/r/46473/#comment193764> "For HTTP endpoints that only work at the leading master, requests made to endpoints at a non-leading master will result in an HTTP 307 redirect to the current leading master." docs/operational-guide.md (line 69) <https://reviews.apache.org/r/46473/#comment193765> Can we link to the endpoint docs for `/state` and `/metrics/snapshot`? - Neil Conway On April 22, 2016, 7:18 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46473/ > --- > > (Updated April 22, 2016, 7:18 a.m.) > > > Review request for mesos, Adam B, Anand Mazumdar, Cody Maloney, Ian Downes, > Neil Conway, and Vinod Kone. > > > Bugs: MESOS-1865 > https://issues.apache.org/jira/browse/MESOS-1865 > > > Repository: mesos > > > Description > --- > > In this changes, we update the `high-availability` and > `operational-guide` documents about master http endpoints redirction > when current master is not the leader. > > > Diffs > - > > docs/high-availability.md b0e744e62081b56e2a24ef5f7304f424424fa3cc > docs/operational-guide.md 5ae7ede3f500380a78364d5c3da2c4cea75e04c5 > > Diff: https://reviews.apache.org/r/46473/diff/ > > > Testing > --- > > > Thanks, > > haosdent huang > >
Re: Review Request 46828: Fixed newline for `--log_dir` flag usage message.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46828/ --- (Updated April 29, 2016, 9:30 a.m.) Review request for mesos and Alexander Rukletsov. Repository: mesos Description (updated) --- See summary. Diffs - src/logging/flags.cpp f64ad5eebc4a14ee07796b1a1c273397bdf46cdd Diff: https://reviews.apache.org/r/46828/diff/ Testing --- Visual inspection Thanks, Neil Conway
Review Request 46828: Fixed newline for `--log_dir` flag usage message.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46828/ --- Review request for mesos and Alexander Rukletsov. Repository: mesos Description --- Fixed newline for `--log_dir` flag usage message. Diffs - src/logging/flags.cpp f64ad5eebc4a14ee07796b1a1c273397bdf46cdd Diff: https://reviews.apache.org/r/46828/diff/ Testing --- Visual inspection Thanks, Neil Conway
Review Request 46827: Replaced CHECK with CHECK_READY.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46827/ --- Review request for mesos and Alexander Rukletsov. Repository: mesos Description --- Also removed some unused header includes. Diffs - src/zookeeper/contender.cpp 4b1cc654cb96d3fd87c5f36c93a8ee1e170bea77 Diff: https://reviews.apache.org/r/46827/diff/ Testing --- make check Thanks, Neil Conway
Review Request 46826: Removed unused header include in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46826/ --- Review request for mesos and Alexander Rukletsov. Repository: mesos Description --- Removed unused header include in libprocess. Diffs - 3rdparty/libprocess/include/process/future.hpp 9a335d199da2e242ba732ae392e66f86c07cd65d Diff: https://reviews.apache.org/r/46826/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 46826: Removed unused header include in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46826/ --- (Updated April 29, 2016, 9:57 a.m.) Review request for mesos and Alexander Rukletsov. Repository: mesos Description (updated) --- See summary. Diffs - 3rdparty/libprocess/include/process/future.hpp 9a335d199da2e242ba732ae392e66f86c07cd65d Diff: https://reviews.apache.org/r/46826/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 46827: Replaced CHECK with CHECK_READY.
> On April 29, 2016, 9:29 a.m., Alexander Rukletsov wrote: > > It would be great to implement `CHECK_NREADY`, `CHECK_NDISCARDED`, then we > > could have replaced `CHECK(!candidacy.isDiscarded());` for consistency. Do > > you want to do it here or follow up with another patch? Seems reasonable to me, but I'd prefer to do this as a separate patch. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46827/#review131066 --- On April 29, 2016, 9:13 a.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46827/ > --- > > (Updated April 29, 2016, 9:13 a.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Repository: mesos > > > Description > --- > > Also removed some unused header includes. > > > Diffs > - > > src/zookeeper/contender.cpp 4b1cc654cb96d3fd87c5f36c93a8ee1e170bea77 > > Diff: https://reviews.apache.org/r/46827/diff/ > > > Testing > --- > > make check > > > Thanks, > > Neil Conway > >
Review Request 46783: Minor clarifications to HA framework guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46783/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Minor clarifications to HA framework guide. Diffs - docs/high-availability-framework-guide.md 62f4c565ded28b0d9b5812eecd53b85ec63c2dec Diff: https://reviews.apache.org/r/46783/diff/ Testing --- Thanks, Neil Conway
Review Request 46782: Removed false claim that TEARDOWN can remove persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46782/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Also make some minor fixes to the scheduler and executor HTTP API docs. Diffs - docs/executor-http-api.md d0e415f4e9f39931ec40f895575acfa7d1f77985 docs/scheduler-http-api.md 6926c272b86a4657f2b6a4836f32722c49da445a Diff: https://reviews.apache.org/r/46782/diff/ Testing --- Thanks, Neil Conway
Review Request 46779: Clarified and improved maintenance docs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46779/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Clarified and improved maintenance docs. Diffs - docs/maintenance.md 1f2b14fcbe86b0a2dc1ad59ce1b17d9d07409930 Diff: https://reviews.apache.org/r/46779/diff/ Testing --- Thanks, Neil Conway
Review Request 46781: Fixed invalid JSON in scheduler HTTP API examples.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46781/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Fixed invalid JSON in scheduler HTTP API examples. Diffs - docs/scheduler-http-api.md 6926c272b86a4657f2b6a4836f32722c49da445a Diff: https://reviews.apache.org/r/46781/diff/ Testing --- Thanks, Neil Conway
Review Request 46780: Clarify that slave checkpoint is always enabled.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46780/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Clarify that slave checkpoint is always enabled. Diffs - include/mesos/mesos.proto 9a180304996895e2e003085690a7dff9ec561e9c Diff: https://reviews.apache.org/r/46780/diff/ Testing --- Thanks, Neil Conway
Re: Review Request 46735: Added support for Authorization information to HELP.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46735/#review131090 --- Fix it, then Ship it! 3rdparty/libprocess/include/process/help.hpp (line 47) <https://reviews.apache.org/r/46735/#comment195035> I might say "authorization requirements and granularity". - Neil Conway On April 27, 2016, 3:33 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46735/ > --- > > (Updated April 27, 2016, 3:33 p.m.) > > > Review request for mesos, Adam B and Alexander Rojas. > > > Repository: mesos > > > Description > --- > > When adding authorization to endpoint we should also > document the authorization behavior in the HELP (and > thereby also in the endpoint documentation). > > > Diffs > - > > 3rdparty/libprocess/include/process/help.hpp > 3f859803f3096d3161fffb6485ce1ce3cb6b04bc > 3rdparty/libprocess/src/help.cpp ff946965360dc688e34a8790b4a5cd8e41fd3d3f > 3rdparty/libprocess/src/logging.cpp > 1cb0f4a1608e15e28d49793d162b62f326cb31df > > Diff: https://reviews.apache.org/r/46735/diff/ > > > Testing > --- > > testing with quota endpoint added (see next review). > > > Thanks, > > Joerg Schad > >
Re: Review Request 46736: Added authorization description to quota endpoint help.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46736/#review131100 --- Ship it! Ship It! - Neil Conway On April 29, 2016, 2:26 p.m., Joerg Schad wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46736/ > --- > > (Updated April 29, 2016, 2:26 p.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, and Alexander Rojas. > > > Repository: mesos > > > Description > --- > > see summary. > > > Diffs > - > > docs/endpoints/master/quota.md 682370f8d7ee44aa858913ba4cee62cb8259e0e6 > src/master/http.cpp 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a > > Diff: https://reviews.apache.org/r/46736/diff/ > > > Testing > --- > > make check (OSX) and re-generated endpoint documentation. > > > Thanks, > > Joerg Schad > >
Re: Review Request 46501: Updated authorization.md to reflect current changes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46501/#review131092 --- docs/authorization.md (line 8) <https://reviews.apache.org/r/46501/#comment195038> First sentence: I would instead say something like, "In Mesos, the authorization subsystem allows the operator to configure the actions that certain principals are allowed to perform. For example, the operator can use authorization to ensure that principal `foo` can only register frameworks in role `bar`, and no other principals can register frameworks in any role." I'd probably add a newline before "A reference" You might want to clarify that (a) ACLs are a property of the local authorizer only; other authorizers might use different concepts for deciding whether a principal is allowed to perform an action, (b) the remainder of the document (?) is specific to the local authorizer; other authorizors might behave differently. docs/authorization.md (line 12) <https://reviews.apache.org/r/46501/#comment195036> "through a" "configured with" docs/authorization.md (line 27) <https://reviews.apache.org/r/46501/#comment195042> Did you remove the "Role vs. Principal" discussion deliberately? If so, why? docs/authorization.md <https://reviews.apache.org/r/46501/#comment195045> The previous text had an example for every ACL action. I think giving an example for every ACL action helps the reader understand how to use ACLs more concretely. docs/authorization.md (line 64) <https://reviews.apache.org/r/46501/#comment195039> "The order in which the rules are defined is important." "the acls" => "the ACLs" docs/authorization.md (line 82) <https://reviews.apache.org/r/46501/#comment195040> "ACLs" docs/authorization.md (line 104) <https://reviews.apache.org/r/46501/#comment195046> You might consider presenting this information using a Markdown table, rather than a list where every element of the list has the same format. docs/authorization.md (line 105) <https://reviews.apache.org/r/46501/#comment195047> "ACLs entry" is imprecise; I would say it is the thing you're describing is the "action", and a single ACL consists of the triple (subject, action, object). docs/authorization.md (line 114) <https://reviews.apache.org/r/46501/#comment195043> For consistency, this needs a newline (``). docs/authorization.md (line 160) <https://reviews.apache.org/r/46501/#comment195049> The actual signature is `process::Future authorized(const authorization::Request& request)`, right? Also, why are we using three back-ticks to denote inline code? AFAIK we should use a single back-tick for that; three back-ticks are for code blocks. I would avoid saying "authorized() should return `false`..." -- authorized() returns a future. I'd say something like "authorized() returns a future that indicates the result of the (asynchronous) authorization operation. If the future is set to `true`, the request was authorized successfully; if it was set to `false`, ..." - Neil Conway On April 28, 2016, 9:56 a.m., Alexander Rojas wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46501/ > --- > > (Updated April 28, 2016, 9:56 a.m.) > > > Review request for mesos, Adam B, Greg Mann, and Neil Conway. > > > Bugs: MESOS-4785 > https://issues.apache.org/jira/browse/MESOS-4785 > > > Repository: mesos > > > Description > --- > > The API of the authorization has been changing constantly over the > last few versions. This patch attempts to update the documentation to > the those changes into account. > > > Diffs > - > > docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 > > Diff: https://reviews.apache.org/r/46501/diff/ > > > Testing > --- > > > Thanks, > > Alexander Rojas > >
Re: Review Request 46781: Fixed invalid JSON in scheduler HTTP API examples.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46781/ --- (Updated April 28, 2016, 3:50 p.m.) Review request for mesos and Jie Yu. Repository: mesos Description --- Fixed invalid JSON in scheduler HTTP API examples. Diffs (updated) - docs/scheduler-http-api.md 6926c272b86a4657f2b6a4836f32722c49da445a Diff: https://reviews.apache.org/r/46781/diff/ Testing --- Thanks, Neil Conway
Re: Review Request 47463: Added documentation for `network/cni` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47463/#review133937 --- docs/cni.md (line 3) <https://reviews.apache.org/r/47463/#comment198598> Phrasing is awkward: who is "we", and when "have we introduced" the CNI isolator? Better would be: "The Mesos `network/cni` isolator allows containers launched using the `MesosContainerizer` to be attached to several different types of IP networks." docs/cni.md (line 40) <https://reviews.apache.org/r/47463/#comment198615> Saying "network namespace" three times in one sentence seems regrettable. docs/cni.md (line 57) <https://reviews.apache.org/r/47463/#comment198617> "and" twice. - Neil Conway On May 18, 2016, 1:31 p.m., Avinash sridharan wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47463/ > --- > > (Updated May 18, 2016, 1:31 p.m.) > > > Review request for mesos, Benjamin Mahler, Jie Yu, Kevin Klues, Neil Conway, > Qian Zhang, and Vinod Kone. > > > Bugs: MESOS-4771 > https://issues.apache.org/jira/browse/MESOS-4771 > > > Repository: mesos > > > Description > --- > > Added documentation for `network/cni` isolator. > > > Diffs > - > > docs/cni.md PRE-CREATION > > Diff: https://reviews.apache.org/r/47463/diff/ > > > Testing > --- > > Build the documentation website and verified the rendering. > > You can review a rendering of the markdown on my github: > https://github.com/asridharan/mesos/blob/MESOS-4771/docs/cni.md > > > Thanks, > > Avinash sridharan > >
Re: Review Request 46888: Documented that `slaveLost` and `executorLost` are unreliable.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46888/ --- (Updated May 12, 2016, 1:36 p.m.) Review request for mesos and Till Toenshoff. Changes --- Tweak summary, description. Summary (updated) - Documented that `slaveLost` and `executorLost` are unreliable. Repository: mesos Description (updated) --- Documented that `slaveLost` and `executorLost` are unreliable. Diffs (updated) - docs/app-framework-development-guide.md 7a5f065695c483bacec664bf36c483604ac2a6a1 include/mesos/scheduler.hpp 859edd2f8b6b13b4a9848e557e2732291e2d4c8b src/java/src/org/apache/mesos/Scheduler.java c7917d43f4a94fd644ab77bfdf9290fa9984f4c6 src/python/interface/src/mesos/interface/__init__.py 1da76ebe577639e8161b16a48a503aa76d568789 Diff: https://reviews.apache.org/r/46888/diff/ Testing --- Thanks, Neil Conway
