Re: Review Request 36049: Added support for modularized Authorizer
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36049/#review93906 --- src/local/local.cpp (line 221) https://reviews.apache.org/r/36049/#comment148332 That's a bit too subtle for me. Proposals, either: - pass the acls to the custom authorizer and let it decide what to do with them - or disallow the presence of both flags, terminate the slave with an error. The latter would also greatly simplify the control flow here. src/master/flags.cpp (line 230) https://reviews.apache.org/r/36049/#comment148333 See above. IMHO better to disallow having both flags simultaneously. If the user gives contradictory instructions, the slave should not start. src/master/flags.cpp (line 421) https://reviews.apache.org/r/36049/#comment148335 See above. src/master/main.cpp (line 305) https://reviews.apache.org/r/36049/#comment148336 See above. - Bernd Mathiske On Aug. 3, 2015, 2:47 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36049/ --- (Updated Aug. 3, 2015, 2:47 a.m.) Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till Toenshoff. Bugs: MESOS-2947 https://issues.apache.org/jira/browse/MESOS-2947 Repository: mesos Description --- Adds and integrates helper classes needed to support an `Authorizer` module. Also adds a flag to the master, allowing the selection of an `Authorizer` module. Diffs - include/mesos/authorizer/authorizer.hpp PRE-CREATION include/mesos/module/authorizer.hpp PRE-CREATION src/Makefile.am e2cbd1524f25b3e3a9419af6bdf8cc6e5022a784 src/authorizer/authorizer.cpp PRE-CREATION src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1 src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 Diff: https://reviews.apache.org/r/36049/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 36908: Added QuotaInfo Protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36908/#review93909 --- Ship it! Looks good to me. src/Makefile.am (lines 182 - 183) https://reviews.apache.org/r/36908/#comment148342 Shall we order these as well a little? src/Makefile.am (line 801) https://reviews.apache.org/r/36908/#comment148338 You really do not want to reorder them, huh? :) src/Makefile.am (line 818) https://reviews.apache.org/r/36908/#comment148343 Not yours but it seems we got some spaces in here that we dont need. Hint, always have a look at your RRs within RB as well and check for such red bars :) - Till Toenshoff On July 29, 2015, 4:59 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36908/ --- (Updated July 29, 2015, 4:59 p.m.) Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-3164 https://issues.apache.org/jira/browse/MESOS-3164 Repository: mesos Description --- Added QuotaInfo Protobuf. Diffs - include/mesos/master/quota.hpp PRE-CREATION include/mesos/master/quota.proto PRE-CREATION src/Makefile.am 0794969b4b147e0f837006f2f2eba6c6b28eb332 Diff: https://reviews.apache.org/r/36908/diff/ Testing --- make distcheck Thanks, Joerg Schad
Re: Review Request 36908: Added QuotaInfo Protobuf.
On Aug. 3, 2015, 2:44 p.m., Bernd Mathiske wrote: include/mesos/master/quota.proto, line 38 https://reviews.apache.org/r/36908/diff/2/?file=1024652#file1024652line38 limit, bound - plural I would suggest to reword: Add upper bounds limit of resources that ... - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36908/#review93908 --- On July 29, 2015, 4:59 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36908/ --- (Updated July 29, 2015, 4:59 p.m.) Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-3164 https://issues.apache.org/jira/browse/MESOS-3164 Repository: mesos Description --- Added QuotaInfo Protobuf. Diffs - include/mesos/master/quota.hpp PRE-CREATION include/mesos/master/quota.proto PRE-CREATION src/Makefile.am 0794969b4b147e0f837006f2f2eba6c6b28eb332 Diff: https://reviews.apache.org/r/36908/diff/ Testing --- make distcheck Thanks, Joerg Schad
Re: Review Request 36048: Updated authorizer to allow for modularized implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36048/#review93907 --- include/mesos/authorizer/authorizer.hpp (line 24) https://reviews.apache.org/r/36048/#comment148334 I just realized that we always uppercase this sentence -- not sure I like it but for consistency, please adapt. - Till Toenshoff On Aug. 3, 2015, 9:47 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36048/ --- (Updated Aug. 3, 2015, 9:47 a.m.) Review request for mesos, Adam B, Alexander Rukletsov, Bernd Mathiske, Kapil Arya, Jan Schlicht, and Till Toenshoff. Bugs: MESOS-2946 https://issues.apache.org/jira/browse/MESOS-2946 Repository: mesos Description --- Splits and updates the original declaration of the `Authorizer` into its interface and a default implementation, the `LocalAuthorizer`. Following the pattern of the modularized `Authenticator`, it generates a default constructor which is required when writing a `TYPED_TEST` in a follow up patch. Additionally, an initialize method has been added, needed for passing in the current ACL definitions as provided via flags. Other changes are just updates to allow for compilation. Diffs - include/mesos/authorizer/authorizer.hpp PRE-CREATION include/mesos/authorizer/authorizer.proto PRE-CREATION include/mesos/mesos.proto cb24125a3f05e0d38fb22e481a15ceb21f882d27 include/mesos/type_utils.hpp 86b37ca1f63e4687af4e86731dceeb1c9c219c5e src/Makefile.am e2cbd1524f25b3e3a9419af6bdf8cc6e5022a784 src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/examples/persistent_volume_framework.cpp c6d6ed337bfca91dc146cb31298cabebdbb13509 src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f src/master/http.cpp 6f5ca02c52462495b84e77525a6c88299746ece2 src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1 src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0 src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b src/tests/mesos.hpp 23d9841c7cfb16abef89a53d29a256a2c5e94b52 src/tests/mesos.cpp f09ef0f99573716de8905f49dcc0c9df20e31ea9 Diff: https://reviews.apache.org/r/36048/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 36908: Added QuotaInfo Protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36908/#review93908 --- include/mesos/master/quota.proto (line 25) https://reviews.apache.org/r/36908/#comment148337 If the guaranteed resource allocation is ALL that QuotaInfo describes, we should rename it. However, reading on, it seems to me that we want QuotaInfo to describe both the minimum AND the limit in the long run. So this should be pointed out right here. The TODO further below comes a bit too late IMHO. include/mesos/master/quota.proto (line 34) https://reviews.apache.org/r/36908/#comment148339 s/should/must s/Resource.role/guaranteed.role s/QuotaInfo.role/the above role include/mesos/master/quota.proto (line 36) https://reviews.apache.org/r/36908/#comment148340 s/guaranteed/guarantees include/mesos/master/quota.proto (line 38) https://reviews.apache.org/r/36908/#comment148341 limit, bound - plural - Bernd Mathiske On July 29, 2015, 9:59 a.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36908/ --- (Updated July 29, 2015, 9:59 a.m.) Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-3164 https://issues.apache.org/jira/browse/MESOS-3164 Repository: mesos Description --- Added QuotaInfo Protobuf. Diffs - include/mesos/master/quota.hpp PRE-CREATION include/mesos/master/quota.proto PRE-CREATION src/Makefile.am 0794969b4b147e0f837006f2f2eba6c6b28eb332 Diff: https://reviews.apache.org/r/36908/diff/ Testing --- make distcheck Thanks, Joerg Schad
Re: Review Request 36913: Added /quota HTTP Endpoint for Quota handling.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36913/#review93975 --- Patch looks great! Reviews applied: [36913] All tests passed. - Mesos ReviewBot On Aug. 3, 2015, 6:09 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36913/ --- (Updated Aug. 3, 2015, 6:09 p.m.) Review request for mesos, Alexander Rukletsov and Bernd Mathiske. Bugs: MESOS-3073 https://issues.apache.org/jira/browse/MESOS-3073 Repository: mesos Description --- Added /quota HTTP Endpoint for Quota handling. Diffs - src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 src/master/quota_handler.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36913/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 32700: Removed FrameworkID from FrameworkState.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32700/#review93988 --- Looks great! Just a couple of questions. Did you run `sudo make check`, since you modified `ROOT_` tests? src/slave/status_update_manager.cpp (lines 209 - 210) https://reviews.apache.org/r/32700/#comment148472 I don't understand this check. A framework without executors will not have a FrameworkInfo? Then why did we even store/recover a FrameworkState? Is this some other upgrade issue? Is a CHECK too strong of an error check? Ditto elsewhere. - Adam B On Aug. 1, 2015, 12:13 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32700/ --- (Updated Aug. 1, 2015, 12:13 p.m.) Review request for mesos and Adam B. Bugs: MESOS-905 https://issues.apache.org/jira/browse/MESOS-905 Repository: mesos Description --- FrameworkState already has FrameworkInfo which will have a valid FrameworkID. NOTE: This patch is only to be merged _ONLY_ after all the dependent patches have shipped, i.e. after 0.23.0 (tracked here: https://issues.apache.org/jira/browse/MESOS-2561) has released. Diffs - src/slave/containerizer/docker.cpp 8f5d302477b216df9ac2f59156304bbc4a96f24b src/slave/containerizer/external_containerizer.cpp d3640e7beaa7a99d7c8938025af879e19cd6b470 src/slave/containerizer/mesos/containerizer.cpp 6d07ff151770bac4eeeb7cd8c9d03f54f2e78ec1 src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1 src/slave/state.hpp cecf200e6e79172af57ae195a73a5161be7e604a src/slave/state.cpp b9f2d8a0d6395b92bd50f1e0927f3ae9fd04b81c src/slave/status_update_manager.cpp 1978ac8ab370f3e20f5c3c803beda468edf31e2c src/tests/containerizer/docker_containerizer_tests.cpp 80ed60e2b0fa39e8302867a7cb6a7388c25f9a40 src/tests/containerizer/mesos_containerizer_tests.cpp 213fa4b0b9c50eba941ef6b52497eb32d539 Diff: https://reviews.apache.org/r/32700/diff/ Testing --- make check. Thanks, Kapil Arya
Re: Review Request 36867: Add labels to FrameworkInfo.
On July 27, 2015, 11:36 p.m., Adam B wrote: Great first patch. Thanks for updating FrameworkInfo on reregistration with the master too! A handful of nits in my first pass. I'll take another look once you've simplified the tests with Kapil's suggestions. Niklas Nielsen wrote: Any updates here? :) Looks like @neilc or somebody resolved some of these issues, but I don't see a new patch revision with the changes. Neil? - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36867/#review93242 --- On July 27, 2015, 6:25 p.m., Neil Conway wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36867/ --- (Updated July 27, 2015, 6:25 p.m.) Review request for mesos, Adam B, Connor Doyle, and Niklas Nielsen. Bugs: MESOS-2841 https://issues.apache.org/jira/browse/MESOS-2841 Repository: mesos Description --- This is intended to support frameworks that want to offer capabilities to the rest of the cluster (e.g., storage or some arbitrary third-party service). We want processes running in the cluster to be able to discover these capabilities; however, we don't want to commit to a fixed set of capabilities or how those capabilities should be represented. Hence, this commit represents this information using freeform key-value pairs, similar to the labels mechanism already in use elsewhere. Jira: MESOS-2841 Diffs - include/mesos/mesos.proto a6748d1cd82238f005c6a49c70d22d095462f1ba src/master/http.cpp 3a1598fad4db03e5f62fd4a6bd26b2bedeee4070 src/master/master.hpp 2c924addfb4c52d3048ee6ded13ce638145cc93f src/tests/fault_tolerance_tests.cpp 7b977f5e8195d9f42b21f36eb36fb156471caa20 src/tests/master_tests.cpp 05c148ee1660b86428afe4eda718b17052743a8c Diff: https://reviews.apache.org/r/36867/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 37046: Merged registerFramework() and reregisterFramework().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37046/ --- (Updated Aug. 3, 2015, 9:47 p.m.) Review request for mesos, Anand Mazumdar and Ben Mahler. Changes --- benm's. NNFR. Bugs: MESOS-3182 https://issues.apache.org/jira/browse/MESOS-3182 Repository: mesos Description --- Merged registerFramework() and reregisterFramework() (and their corresponding continuations) into subscribe() (and _subscribe()). Diffs (updated) - src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 Diff: https://reviews.apache.org/r/37046/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36847: Added HTTP Delete Method.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36847/#review93998 --- Patch looks great! Reviews applied: [36847] All tests passed. - Mesos ReviewBot On Aug. 3, 2015, 6:45 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36847/ --- (Updated Aug. 3, 2015, 6:45 p.m.) Review request for mesos, Alexander Rukletsov and Bernd Mathiske. Bugs: MESOS-3152 https://issues.apache.org/jira/browse/MESOS-3152 Repository: mesos Description --- see summary. Diffs - 3rdparty/libprocess/include/process/http.hpp b8d930058aa3ae9d21ae0f7b420d49f2448f84b1 3rdparty/libprocess/src/http.cpp 4dcbd74b894b483c4d166c23fac55ac8dba75166 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3 Diff: https://reviews.apache.org/r/36847/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/#review93994 --- Patch looks great! Reviews applied: [36999, 36646, 36404] All tests passed. - Mesos ReviewBot On Aug. 3, 2015, 6:30 p.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 3, 2015, 6:30 p.m.) Review request for mesos, Joris Van Remoortere and Joseph Wu. Repository: mesos Description --- JIRA: https://issues.apache.org/jira/browse/MESOS-2964 Diffs - 3rdparty/libprocess/include/process/io.hpp 975923f40f82357f31b89428f24d01df6a8ac9fc 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 3rdparty/libprocess/src/tests/io_tests.cpp c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 37054: Moved ContainerInfo::Image definition to the top level.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37054/#review93997 --- Ship it! Ship It! - Timothy Chen On Aug. 3, 2015, 11:09 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37054/ --- (Updated Aug. 3, 2015, 11:09 p.m.) Review request for mesos, Benjamin Hindman, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Moved ContainerInfo::Image definition to the top level. Motivation is captured in this doc: https://docs.google.com/document/d/1n2emC2ruTMur5nURvLgGYJxuP-tgBwgLDrmg_17QSmA/edit# Diffs - include/mesos/mesos.proto a6748d1cd82238f005c6a49c70d22d095462f1ba src/slave/containerizer/mesos/containerizer.hpp 8851d30af56b4f9fb95450ac1f42ab550e3df9ff src/slave/containerizer/mesos/containerizer.cpp 6d07ff151770bac4eeeb7cd8c9d03f54f2e78ec1 src/slave/containerizer/provisioner.hpp cb4d511e8189b65df9b9803f23812dd98edc44ac src/slave/containerizer/provisioner.cpp df52e36b23ad3cd28f50e96865d0b163cc245cb2 src/tests/containerizer/mesos_containerizer_tests.cpp 213fa4b0b9c50eba941ef6b52497eb32d539 Diff: https://reviews.apache.org/r/37054/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 37054: Moved ContainerInfo::Image definition to the top level.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37054/#review93996 --- include/mesos/mesos.proto (line 1194) https://reviews.apache.org/r/37054/#comment148490 I think it's worth mentioninig, specifically provided for provisioners and the Mesos containerizer. - Timothy Chen On Aug. 3, 2015, 11:09 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37054/ --- (Updated Aug. 3, 2015, 11:09 p.m.) Review request for mesos, Benjamin Hindman, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Moved ContainerInfo::Image definition to the top level. Motivation is captured in this doc: https://docs.google.com/document/d/1n2emC2ruTMur5nURvLgGYJxuP-tgBwgLDrmg_17QSmA/edit# Diffs - include/mesos/mesos.proto a6748d1cd82238f005c6a49c70d22d095462f1ba src/slave/containerizer/mesos/containerizer.hpp 8851d30af56b4f9fb95450ac1f42ab550e3df9ff src/slave/containerizer/mesos/containerizer.cpp 6d07ff151770bac4eeeb7cd8c9d03f54f2e78ec1 src/slave/containerizer/provisioner.hpp cb4d511e8189b65df9b9803f23812dd98edc44ac src/slave/containerizer/provisioner.cpp df52e36b23ad3cd28f50e96865d0b163cc245cb2 src/tests/containerizer/mesos_containerizer_tests.cpp 213fa4b0b9c50eba941ef6b52497eb32d539 Diff: https://reviews.apache.org/r/37054/diff/ Testing --- make check Thanks, Jie Yu
Review Request 37065: Fixed MemIsolatorTest failure on OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/ --- Review request for mesos, Benjamin Hindman and switched to 'mcypark'. Bugs: MESOS-3197 https://issues.apache.org/jira/browse/MESOS-3197 Repository: mesos Description --- See summary. Diffs - src/tests/containerizer/memory_test_helper.cpp 5e40b747f4266e7532baf8fd02ea5db0955124d2 Diff: https://reviews.apache.org/r/37065/diff/ Testing --- make check Thanks, Artem Harutyunyan
Re: Review Request 36814: Fill executor_id in state.json when task is run in CommandExecutor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36814/#review93995 --- Ship it! LGTM. I like the idea of not setting the executorId=taskId in the actual TaskInfo/Task, since that could confuse other logic downstream that expects the executor/executorId to be empty for command executors. However, since this is exposed in state.json, there might be external tools/scripts that expect the same. Now they can just check if executorId==taskId, assuming there are no custom executors using the same naming scheme. Could you please add a note to docs/upgrades.md mentioning the API change, in case anybody does depend on the old behavior? Does this actually solve the problem of: The webui is broken for command executors because the master does not know the executor ID for the tasks using a command executor.? @bmahler do you have any further thoughts/questions on this simple change? - Adam B On Aug. 2, 2015, 12:57 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36814/ --- (Updated Aug. 2, 2015, 12:57 a.m.) Review request for mesos, Adam B and Ben Mahler. Bugs: MESOS-527 https://issues.apache.org/jira/browse/MESOS-527 Repository: mesos Description --- Fill executor_id in state.json when task is run in CommandExecutor. Diffs - src/common/http.cpp a74c51d9392d0b0a67d51a0143eb314cfca11245 src/tests/common/http_tests.cpp 38d062b2b4062e0a2fc912bad0cc2d73339eb66e Diff: https://reviews.apache.org/r/36814/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 37045: Convert Linux perf sampler to use process:await().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/#review93968 --- Looks pretty good, thanks Paul! Just a couple bits of feedback around failure handling and use of .then. src/linux/perf.cpp (line 176) https://reviews.apache.org/r/37045/#comment148432 Why the change here? Mind reverting this? src/linux/perf.cpp (lines 292 - 294) https://reviews.apache.org/r/37045/#comment148435 Hm.. why the explicit Futurestring wrapping here? Also, mind lining things up on the open paren here? src/linux/perf.cpp (lines 295 - 297) https://reviews.apache.org/r/37045/#comment148440 Couple of things here: (1) .then performs composition, so only if the future succeeds (ready) will the callback be invoked, this means you don't need the outer future type in the callback: ``` .then([=](const std::tuple FutureOptionint, Futurestring, Futurestring results) - FutureNothing ``` But since we don't care about composition here, we should just use .onReady, which will avoid the need to return Failures to satisfy the FutureNothing return type, we can just have a void continuation. (2) Any reason to switch to a lambda for this? Note that you need to at least 'defer' to this lambda, as before. Otherwise, your continuation will execute without locking the actor! src/linux/perf.cpp (lines 299 - 305) https://reviews.apache.org/r/37045/#comment148441 Per the above comments, this will never happen since .then is passing a tuple directly, not a Future. src/linux/perf.cpp (lines 306 - 310) https://reviews.apache.org/r/37045/#comment148442 We can't call .get() on each individual future until we've validated that it is ready. How about pulling out the things we're interested in? ``` FutureOptionint status = std::get0(results); Futurestring output = std::get1(results); ``` Then validating that these are not failed. - Ben Mahler On Aug. 3, 2015, 9:10 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/ --- (Updated Aug. 3, 2015, 9:10 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Convert Linux perf sampler to use process:await(). Diffs - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/37045/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36402: Adding 'Accept' header in request
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review93970 --- Is this ready to review? Mind updating the 'issues' accordingly? - Ben Mahler On Aug. 3, 2015, 8:19 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated Aug. 3, 2015, 8:19 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs - 3rdparty/libprocess/include/process/http.hpp b8d9300 3rdparty/libprocess/src/http.cpp 4dcbd74 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5 Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36946: Factored out the pattern for URL generation in a fetcher test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36946/#review93878 --- Ship it! Ship It! - Bernd Mathiske On July 31, 2015, 8:23 a.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36946/ --- (Updated July 31, 2015, 8:23 a.m.) Review request for mesos, Bernd Mathiske and Klaus Ma. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Factored out the pattern for URL generation in (another)fetcher test. Diffs - src/tests/fetcher_tests.cpp 3ded3c037bdfe7095aa15503d81a8d2ee9d420df Diff: https://reviews.apache.org/r/36946/diff/ Testing --- GTEST_FILTER='FetcherTest.OSNetUriSpaceTest' make check Thanks, Artem Harutyunyan
Re: Review Request 36773: Implemented a LRU entry selection criteria for cache eviction.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36773/ --- (Updated Aug. 3, 2015, 10:43 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3112 https://issues.apache.org/jira/browse/MESOS-3112 Repository: mesos Description --- A linked list is used to keep cache entries in LRU-to-MRU order. Each time an existing cache entry is requested, it is moved to the back of the list. During cache eviction entries are removed from the front of the list until enough cache space can be freed. Diffs (updated) - docs/fetcher-cache-internals.md 327cbc3074bcc110e1250e2151dd5401ccaadb4b src/slave/containerizer/fetcher.hpp 17225072ba5c1c9a7209f2923bcf562fcb76201f src/slave/containerizer/fetcher.cpp e030deabd5e749100cbccabb256dbd4af8b2fe58 src/tests/fetcher_cache_tests.cpp 4e1d348c412c0cec6a73d6e43f2c7124db1f3bfe Diff: https://reviews.apache.org/r/36773/diff/ Testing --- make check Thanks, Jan Schlicht
Re: Review Request 36773: Implemented a LRU entry selection criteria for cache eviction.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36773/#review93882 --- Patch looks great! Reviews applied: [36773] All tests passed. - Mesos ReviewBot On Aug. 3, 2015, 8:43 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36773/ --- (Updated Aug. 3, 2015, 8:43 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3112 https://issues.apache.org/jira/browse/MESOS-3112 Repository: mesos Description --- A linked list is used to keep cache entries in LRU-to-MRU order. Each time an existing cache entry is requested, it is moved to the back of the list. During cache eviction entries are removed from the front of the list until enough cache space can be freed. Diffs - docs/fetcher-cache-internals.md 327cbc3074bcc110e1250e2151dd5401ccaadb4b src/slave/containerizer/fetcher.hpp 17225072ba5c1c9a7209f2923bcf562fcb76201f src/slave/containerizer/fetcher.cpp e030deabd5e749100cbccabb256dbd4af8b2fe58 src/tests/fetcher_cache_tests.cpp 4e1d348c412c0cec6a73d6e43f2c7124db1f3bfe Diff: https://reviews.apache.org/r/36773/diff/ Testing --- make check Thanks, Jan Schlicht
Re: Review Request 36773: Implemented a LRU entry selection criteria for cache eviction.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36773/#review93886 --- Ship it! Ship It! - Joerg Schad On Aug. 3, 2015, 8:43 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36773/ --- (Updated Aug. 3, 2015, 8:43 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3112 https://issues.apache.org/jira/browse/MESOS-3112 Repository: mesos Description --- A linked list is used to keep cache entries in LRU-to-MRU order. Each time an existing cache entry is requested, it is moved to the back of the list. During cache eviction entries are removed from the front of the list until enough cache space can be freed. Diffs - docs/fetcher-cache-internals.md 327cbc3074bcc110e1250e2151dd5401ccaadb4b src/slave/containerizer/fetcher.hpp 17225072ba5c1c9a7209f2923bcf562fcb76201f src/slave/containerizer/fetcher.cpp e030deabd5e749100cbccabb256dbd4af8b2fe58 src/tests/fetcher_cache_tests.cpp 4e1d348c412c0cec6a73d6e43f2c7124db1f3bfe Diff: https://reviews.apache.org/r/36773/diff/ Testing --- make check Thanks, Jan Schlicht
Re: Review Request 36048: Updated authorizer to allow for modularized implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36048/#review93884 --- Ship it! I think this looks ready for landing after fixing the remaining minor issues. src/authorizer/authorizer.hpp (line 60) https://reviews.apache.org/r/36048/#comment148300 Add a comment please on what we do with this flag and why a Once or alike won't do. src/authorizer/authorizer.cpp (line 236) https://reviews.apache.org/r/36048/#comment148301 s/LocalAuthorizer/local authorizer/ src/local/local.cpp (line 224) https://reviews.apache.org/r/36048/#comment148302 Seems a failure to instantiate the authorizer should not be connected to the settings on --acls - we should get rid of the (see --acls flag), no? src/tests/cluster.hpp (line 60) https://reviews.apache.org/r/36048/#comment148305 I think this should move up, a couple of lines, above files/files.hpp src/tests/cluster.hpp (line 359) https://reviews.apache.org/r/36048/#comment148306 --acls has no influence on the instantiation as commented above, please remove that part from the message. - Till Toenshoff On Aug. 3, 2015, 9:47 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36048/ --- (Updated Aug. 3, 2015, 9:47 a.m.) Review request for mesos, Adam B, Alexander Rukletsov, Bernd Mathiske, Kapil Arya, Jan Schlicht, and Till Toenshoff. Bugs: MESOS-2946 https://issues.apache.org/jira/browse/MESOS-2946 Repository: mesos Description --- Splits and updates the original declaration of the `Authorizer` into its interface and a default implementation, the `LocalAuthorizer`. Following the pattern of the modularized `Authenticator`, it generates a default constructor which is required when writing a `TYPED_TEST` in a follow up patch. Additionally, an initialize method has been added, needed for passing in the current ACL definitions as provided via flags. Other changes are just updates to allow for compilation. Diffs - include/mesos/authorizer/authorizer.hpp PRE-CREATION include/mesos/authorizer/authorizer.proto PRE-CREATION include/mesos/mesos.proto cb24125a3f05e0d38fb22e481a15ceb21f882d27 include/mesos/type_utils.hpp 86b37ca1f63e4687af4e86731dceeb1c9c219c5e src/Makefile.am e2cbd1524f25b3e3a9419af6bdf8cc6e5022a784 src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/examples/persistent_volume_framework.cpp c6d6ed337bfca91dc146cb31298cabebdbb13509 src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f src/master/http.cpp 6f5ca02c52462495b84e77525a6c88299746ece2 src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1 src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0 src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b src/tests/mesos.hpp 23d9841c7cfb16abef89a53d29a256a2c5e94b52 src/tests/mesos.cpp f09ef0f99573716de8905f49dcc0c9df20e31ea9 Diff: https://reviews.apache.org/r/36048/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 36847: Added HTTP Delete Method.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36847/#review93880 --- 3rdparty/libprocess/include/process/http.hpp (line 736) https://reviews.apache.org/r/36847/#comment148297 (We have discussed this naming choice at some length offline. Here is how I see it.) If I find a call site somewhere that reads http:del(...) I will probably wonder what this is and will look it up. Not good enough IMHO. Cryptic. If instead I read http::sendDeleteRequest(), it is immediately clear to me what this call does. True, this makes the API unbalanced, since the other calls are simply called get and put. Well, they never should have! Even if we never rename the other calls, I favor readability at call sites over superficially consistent naming across the API. 2c - Bernd Mathiske On July 29, 2015, 6:39 a.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36847/ --- (Updated July 29, 2015, 6:39 a.m.) Review request for mesos, Alexander Rukletsov and Bernd Mathiske. Bugs: MESOS-3152 https://issues.apache.org/jira/browse/MESOS-3152 Repository: mesos Description --- see summary. Diffs - 3rdparty/libprocess/include/process/http.hpp 9faed55247a3ccd629db7b85dbf31d3117e120e9 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f 3rdparty/libprocess/src/tests/http_tests.cpp 01f243cd9c46e162c16e9bb452a846faf31d1445 Diff: https://reviews.apache.org/r/36847/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 36048: Updated authorizer to allow for modularized implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36048/#review93912 --- src/authorizer/authorizer.cpp (line 209) https://reviews.apache.org/r/36048/#comment148352 I stumbled over this syntax. While it's valid C++11, I'd suggest using `Authorizer* local = new LocalAuthorizer` as that's easier to grok. - Jan Schlicht On Aug. 3, 2015, 11:47 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36048/ --- (Updated Aug. 3, 2015, 11:47 a.m.) Review request for mesos, Adam B, Alexander Rukletsov, Bernd Mathiske, Kapil Arya, Jan Schlicht, and Till Toenshoff. Bugs: MESOS-2946 https://issues.apache.org/jira/browse/MESOS-2946 Repository: mesos Description --- Splits and updates the original declaration of the `Authorizer` into its interface and a default implementation, the `LocalAuthorizer`. Following the pattern of the modularized `Authenticator`, it generates a default constructor which is required when writing a `TYPED_TEST` in a follow up patch. Additionally, an initialize method has been added, needed for passing in the current ACL definitions as provided via flags. Other changes are just updates to allow for compilation. Diffs - include/mesos/authorizer/authorizer.hpp PRE-CREATION include/mesos/authorizer/authorizer.proto PRE-CREATION include/mesos/mesos.proto cb24125a3f05e0d38fb22e481a15ceb21f882d27 include/mesos/type_utils.hpp 86b37ca1f63e4687af4e86731dceeb1c9c219c5e src/Makefile.am e2cbd1524f25b3e3a9419af6bdf8cc6e5022a784 src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/examples/persistent_volume_framework.cpp c6d6ed337bfca91dc146cb31298cabebdbb13509 src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f src/master/http.cpp 6f5ca02c52462495b84e77525a6c88299746ece2 src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1 src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0 src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b src/tests/mesos.hpp 23d9841c7cfb16abef89a53d29a256a2c5e94b52 src/tests/mesos.cpp f09ef0f99573716de8905f49dcc0c9df20e31ea9 Diff: https://reviews.apache.org/r/36048/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 36929: Fixed a few issues in test launcher header.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36929/#review93917 --- Ship it! Ship It! - Vinod Kone On July 31, 2015, 10:23 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36929/ --- (Updated July 31, 2015, 10:23 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- Fixed a few issues in test launcher header. Diffs - src/tests/containerizer/launcher.hpp b80e84196f8b494e6e5a3c96c86f63fe432a7bb0 Diff: https://reviews.apache.org/r/36929/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 36847: Added HTTP Delete Method.
On Aug. 3, 2015, 5:47 p.m., Marco Massenzio wrote: 3rdparty/libprocess/src/http.cpp, lines 927-929 https://reviews.apache.org/r/36847/diff/4/?file=1023561#file1023561line927 I would add a statement in the (javadoc?) method's documentation, to the effect that a query or fragment parts should *not* be used in this request, and the outcome of doing so are undefined. This would also follow the principle of least surprise for the callers of this method (who would also not need to go reverse engineering the code to figure out why their code ain't working...) I created MESOS-3163 to also handle/discuss this across the other methods. is that ok for you? - Joerg --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36847/#review93923 --- On July 29, 2015, 1:39 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36847/ --- (Updated July 29, 2015, 1:39 p.m.) Review request for mesos, Alexander Rukletsov and Bernd Mathiske. Bugs: MESOS-3152 https://issues.apache.org/jira/browse/MESOS-3152 Repository: mesos Description --- see summary. Diffs - 3rdparty/libprocess/include/process/http.hpp 9faed55247a3ccd629db7b85dbf31d3117e120e9 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f 3rdparty/libprocess/src/tests/http_tests.cpp 01f243cd9c46e162c16e9bb452a846faf31d1445 Diff: https://reviews.apache.org/r/36847/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 36847: Added HTTP Delete Method.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36847/ --- (Updated Aug. 3, 2015, 6:45 p.m.) Review request for mesos, Alexander Rukletsov and Bernd Mathiske. Changes --- Addressed comments. Bugs: MESOS-3152 https://issues.apache.org/jira/browse/MESOS-3152 Repository: mesos Description --- see summary. Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp b8d930058aa3ae9d21ae0f7b420d49f2448f84b1 3rdparty/libprocess/src/http.cpp 4dcbd74b894b483c4d166c23fac55ac8dba75166 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3 Diff: https://reviews.apache.org/r/36847/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 36908: Added QuotaInfo Protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36908/ --- (Updated Aug. 3, 2015, 7:01 p.m.) Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-3164 https://issues.apache.org/jira/browse/MESOS-3164 Repository: mesos Description --- Added QuotaInfo Protobuf. Diffs (updated) - include/mesos/master/quota.hpp PRE-CREATION include/mesos/master/quota.proto PRE-CREATION src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec Diff: https://reviews.apache.org/r/36908/diff/ Testing --- make distcheck Thanks, Joerg Schad
Re: Review Request 36956: Created a test abstraction for preparing test rootfs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36956/#review93933 --- Patch looks great! Reviews applied: [36929, 36930, 36954, 36956] All tests passed. - Mesos ReviewBot On Aug. 3, 2015, 5:18 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36956/ --- (Updated Aug. 3, 2015, 5:18 p.m.) Review request for mesos, Ian Downes, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Bugs: MESOS-3179 https://issues.apache.org/jira/browse/MESOS-3179 Repository: mesos Description --- Created a test abstraction for preparing test rootfs. Diffs - src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec src/tests/containerizer/launch_tests.cpp 73c8c5fa17936b1bab4817e9f1e691144e87c35f src/tests/containerizer/rootfs.hpp PRE-CREATION Diff: https://reviews.apache.org/r/36956/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 3, 2015, 11:30 a.m.) Review request for mesos, Joris Van Remoortere and Joseph Wu. Changes --- Missing commas. Repository: mesos Description --- JIRA: https://issues.apache.org/jira/browse/MESOS-2964 Diffs (updated) - 3rdparty/libprocess/include/process/io.hpp 975923f40f82357f31b89428f24d01df6a8ac9fc 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 3rdparty/libprocess/src/tests/io_tests.cpp c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36987: Extend 'getFormValue' - 'getFormValues' to parse input uniformly.
On Aug. 3, 2015, 6:43 p.m., Ben Mahler wrote: src/master/http.cpp, lines 361-362 https://reviews.apache.org/r/36987/diff/2/?file=1026437#file1026437line361 Do we even want to keep this? Note that the 'observe' has never been used. I've found this to be a strange helper in the first place, how would we explain what this does in a comment? Note that the 'observe' has never been used. What do you mean? It looks like `/observe` is an exposed endpoint that calls `observe`? I've found this to be a strange helper in the first place, how would we explain what this does in a comment? I should've accompanied it with a comment. My view of its functionality is something like: ``` Takes an HTTP request and the expected list of keys, tries to return the map of expected keys to their corresponding values. It returns an Error if we failed to parse the request, if an expected key was not provided, or if the corresponding value is an empty string. Otherwise, the resulting hashmap contains the mapping from expected keys to provided values. ``` I think it's a useful helper function for capturing the above functionality as well as to facilitate providing consistent error messages. (e.g. for `/observe` we returned Missing value for 'monitor'., whereas for `/teardown` we returned Missing 'frameworkId' query parameter) - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36987/#review93942 --- On July 31, 2015, 9:56 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36987/ --- (Updated July 31, 2015, 9:56 p.m.) Review request for mesos and Jie Yu. Repository: mesos Description --- See summary. Diffs - src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66 Diff: https://reviews.apache.org/r/36987/diff/ Testing --- `make check` Thanks, Michael Park
Re: Review Request 36911: Removed unnecessary using directive.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36911/#review93919 --- Ship it! Ship It! - Marco Massenzio On Aug. 3, 2015, 2:58 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36911/ --- (Updated Aug. 3, 2015, 2:58 p.m.) Review request for mesos, Marco Massenzio and Till Toenshoff. Repository: mesos Description --- Removed unnecessary using directive. Diffs - src/master/master.cpp c584cb9f5aeb6806657059a3204ce1c680d4214a Diff: https://reviews.apache.org/r/36911/diff/ Testing --- make check (clang on Mac OS 10.10.4) Thanks, Alexander Rukletsov
Re: Review Request 36819: Use setup.py in python cli package.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36819/#review93931 --- Ship it! This looks good to me. Provided that we test it on a couple of other platforms (CentOS 7.x and Ubuntu 14.04 would be good) I'm fine with it. You still need a shepher to commit this, though - I can't. - Marco Massenzio On Aug. 1, 2015, 10:09 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36819/ --- (Updated Aug. 1, 2015, 10:09 a.m.) Review request for mesos, Benjamin Hindman and Marco Massenzio. Bugs: MESOS-3149 https://issues.apache.org/jira/browse/MESOS-3149 Repository: mesos Description --- Use setup.py in python cli package. Diffs - Makefile.am f8e958d376efa55ace7c5727a31b8e747403641e bin/mesos.sh.in 5cbeac4330a9f45fc6d54b8c2d383f48e4098f95 configure.ac 546c9bbf775a4ef481fafb3a58c85c6d80e19500 src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec src/cli/python/mesos/__init__.py 028b0d27fb193bac96f2a6a3201ee4cc8fd369ef src/cli/python/mesos/cli.py src/cli/python/mesos/futures.py src/cli/python/mesos/http.py src/python/cli/src/mesos/__init__.py PRE-CREATION src/python/interface/src/mesos/__init__.py f48ad10528712b2b8960f1863d156b88ed1ce311 src/python/native/src/mesos/__init__.py f48ad10528712b2b8960f1863d156b88ed1ce311 src/python/protocol/src/mesos/__init__.py f48ad10528712b2b8960f1863d156b88ed1ce311 Diff: https://reviews.apache.org/r/36819/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 36956: Created a test abstraction for preparing test rootfs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36956/ --- (Updated Aug. 3, 2015, 5:18 p.m.) Review request for mesos, Ian Downes, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Changes --- Added the missing header in the makefile. Bugs: MESOS-3179 https://issues.apache.org/jira/browse/MESOS-3179 Repository: mesos Description --- Created a test abstraction for preparing test rootfs. Diffs (updated) - src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec src/tests/containerizer/launch_tests.cpp 73c8c5fa17936b1bab4817e9f1e691144e87c35f src/tests/containerizer/rootfs.hpp PRE-CREATION Diff: https://reviews.apache.org/r/36956/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated Aug. 3, 2015, 10:44 a.m.) Review request for mesos, Joris Van Remoortere and Joseph Wu. Repository: mesos Description --- JIRA: https://issues.apache.org/jira/browse/MESOS-2964 Diffs (updated) - 3rdparty/libprocess/include/process/io.hpp 975923f40f82357f31b89428f24d01df6a8ac9fc 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 3rdparty/libprocess/src/tests/io_tests.cpp c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36847: Added HTTP Delete Method.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36847/#review93923 --- 3rdparty/libprocess/include/process/http.hpp (line 736) https://reviews.apache.org/r/36847/#comment148360 please don't use abbreviations. `delete()` is the HTTP method name. I'm almost sure it violates our style guide (and I'm positive it violates Google's). 3rdparty/libprocess/include/process/http.hpp (line 741) https://reviews.apache.org/r/36847/#comment148361 I would so much love to see the javadoc used, so this shows up in the online documentation (and in my IDE when I hover over the callsite); instead of having to go fisthing for the source file and find this documentation 3rdparty/libprocess/src/http.cpp (lines 927 - 929) https://reviews.apache.org/r/36847/#comment148363 I would add a statement in the (javadoc?) method's documentation, to the effect that a query or fragment parts should *not* be used in this request, and the outcome of doing so are undefined. This would also follow the principle of least surprise for the callers of this method (who would also not need to go reverse engineering the code to figure out why their code ain't working...) - Marco Massenzio On July 29, 2015, 1:39 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36847/ --- (Updated July 29, 2015, 1:39 p.m.) Review request for mesos, Alexander Rukletsov and Bernd Mathiske. Bugs: MESOS-3152 https://issues.apache.org/jira/browse/MESOS-3152 Repository: mesos Description --- see summary. Diffs - 3rdparty/libprocess/include/process/http.hpp 9faed55247a3ccd629db7b85dbf31d3117e120e9 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f 3rdparty/libprocess/src/tests/http_tests.cpp 01f243cd9c46e162c16e9bb452a846faf31d1445 Diff: https://reviews.apache.org/r/36847/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 36847: Added HTTP Delete Method.
On Aug. 3, 2015, 5:47 p.m., Marco Massenzio wrote: 3rdparty/libprocess/include/process/http.hpp, line 736 https://reviews.apache.org/r/36847/diff/4/?file=1023560#file1023560line736 please don't use abbreviations. `delete()` is the HTTP method name. I'm almost sure it violates our style guide (and I'm positive it violates Google's). Using delete() unfortunately violates c++ list of reserved keyword Previously tried something like sendDeleteRequest which people found to long/verbose and inconsistent... - Joerg --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36847/#review93923 --- On July 29, 2015, 1:39 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36847/ --- (Updated July 29, 2015, 1:39 p.m.) Review request for mesos, Alexander Rukletsov and Bernd Mathiske. Bugs: MESOS-3152 https://issues.apache.org/jira/browse/MESOS-3152 Repository: mesos Description --- see summary. Diffs - 3rdparty/libprocess/include/process/http.hpp 9faed55247a3ccd629db7b85dbf31d3117e120e9 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f 3rdparty/libprocess/src/tests/http_tests.cpp 01f243cd9c46e162c16e9bb452a846faf31d1445 Diff: https://reviews.apache.org/r/36847/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 36913: Added /quota HTTP Endpoint for Quota handling.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36913/ --- (Updated Aug. 3, 2015, 6:09 p.m.) Review request for mesos, Alexander Rukletsov and Bernd Mathiske. Changes --- Moved QuotaHandler declaration into master.hpp in order to avoid cyclic dependency (similar to http class). Bugs: MESOS-3073 https://issues.apache.org/jira/browse/MESOS-3073 Repository: mesos Description --- Added /quota HTTP Endpoint for Quota handling. Diffs (updated) - src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 src/master/quota_handler.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36913/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 36913: Added /quota HTTP Endpoint for Quota handling.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36913/ --- (Updated Aug. 3, 2015, 11:55 a.m.) Review request for mesos, Alexander Rukletsov and Bernd Mathiske. Changes --- Added dummy quota_handler.cpp in order facilitate colaborative work. Bugs: MESOS-3073 https://issues.apache.org/jira/browse/MESOS-3073 Repository: mesos Description --- Added /quota HTTP Endpoint for Quota handling. Diffs (updated) - src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 src/master/quota_handler.hpp PRE-CREATION src/master/quota_handler.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36913/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 36913: Added /quota HTTP Endpoint for Quota handling.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36913/#review93890 --- src/master/http.cpp (lines 517 - 521) https://reviews.apache.org/r/36913/#comment148310 A minor nit: let's sort them as they appear in the RFC: GET POST PUT DELETE - Alexander Rukletsov On July 29, 2015, 8:48 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36913/ --- (Updated July 29, 2015, 8:48 p.m.) Review request for mesos, Alexander Rukletsov and Bernd Mathiske. Bugs: MESOS-3073 https://issues.apache.org/jira/browse/MESOS-3073 Repository: mesos Description --- Added /quota HTTP Endpoint for Quota handling. Diffs - src/Makefile.am 0794969b4b147e0f837006f2f2eba6c6b28eb332 src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f src/master/master.hpp 2331173fb9aeca77227fb09b899cb123eb205b4b src/master/master.cpp c584cb9f5aeb6806657059a3204ce1c680d4214a src/master/quota_handler.hpp PRE-CREATION Diff: https://reviews.apache.org/r/36913/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 36773: Implemented a LRU entry selection criteria for cache eviction.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36773/#review93889 --- Ship it! Ship It! - Bernd Mathiske On Aug. 3, 2015, 1:43 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36773/ --- (Updated Aug. 3, 2015, 1:43 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3112 https://issues.apache.org/jira/browse/MESOS-3112 Repository: mesos Description --- A linked list is used to keep cache entries in LRU-to-MRU order. Each time an existing cache entry is requested, it is moved to the back of the list. During cache eviction entries are removed from the front of the list until enough cache space can be freed. Diffs - docs/fetcher-cache-internals.md 327cbc3074bcc110e1250e2151dd5401ccaadb4b src/slave/containerizer/fetcher.hpp 17225072ba5c1c9a7209f2923bcf562fcb76201f src/slave/containerizer/fetcher.cpp e030deabd5e749100cbccabb256dbd4af8b2fe58 src/tests/fetcher_cache_tests.cpp 4e1d348c412c0cec6a73d6e43f2c7124db1f3bfe Diff: https://reviews.apache.org/r/36773/diff/ Testing --- make check Thanks, Jan Schlicht
Re: Review Request 36049: Added support for modularized Authorizer
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36049/#review93898 --- Ship it! Ship It! - Till Toenshoff On Aug. 3, 2015, 9:47 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36049/ --- (Updated Aug. 3, 2015, 9:47 a.m.) Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till Toenshoff. Bugs: MESOS-2947 https://issues.apache.org/jira/browse/MESOS-2947 Repository: mesos Description --- Adds and integrates helper classes needed to support an `Authorizer` module. Also adds a flag to the master, allowing the selection of an `Authorizer` module. Diffs - include/mesos/authorizer/authorizer.hpp PRE-CREATION include/mesos/module/authorizer.hpp PRE-CREATION src/Makefile.am e2cbd1524f25b3e3a9419af6bdf8cc6e5022a784 src/authorizer/authorizer.cpp PRE-CREATION src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1 src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 Diff: https://reviews.apache.org/r/36049/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 36050: Added test authorizer module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36050/#review93899 --- Ship it! src/tests/authorization_tests.cpp (line 25) https://reviews.apache.org/r/36050/#comment148316 This include should go above the process/future.hpp - Till Toenshoff On Aug. 3, 2015, 9:47 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36050/ --- (Updated Aug. 3, 2015, 9:47 a.m.) Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till Toenshoff. Bugs: MESOS-2947 https://issues.apache.org/jira/browse/MESOS-2947 Repository: mesos Description --- Adds a test authorizer module. Updates the authorization tests in order to perform typed tests on the default authorizer implementation and on the test authorizer module. Diffs - src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 src/examples/test_authorizer_module.cpp PRE-CREATION src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 src/tests/module.hpp 03756a2536ca3e662ba422e96d121a6c39bb8c84 src/tests/module.cpp 61d4753f0f098005f56dd4a24984e30405c32558 Diff: https://reviews.apache.org/r/36050/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 36048: Updated authorizer to allow for modularized implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36048/#review93900 --- include/mesos/authorizer/authorizer.hpp (line 34) https://reviews.apache.org/r/36048/#comment148318 Note to all: this interface is in flux. It WILL change soon. I could start criticizing it here, but it is consistent with existing code and we should make the interface content a subsequent, separate issue. Here, we focus on the modularization per se. include/mesos/authorizer/authorizer.hpp (line 37) https://reviews.apache.org/r/36048/#comment148319 Can we have doxygen for this whole interface and every item in it, please? include/mesos/authorizer/authorizer.proto (line 28) https://reviews.apache.org/r/36048/#comment148317 What is local authorization? Link or explanation, please! src/authorizer/authorizer.cpp (line 236) https://reviews.apache.org/r/36048/#comment148320 s/ACL's/ACLs src/master/main.cpp (line 307) https://reviews.apache.org/r/36048/#comment148323 --acls has no influence on the instantiation as commented above, please remove that part from the message. src/master/master.hpp (line 77) https://reviews.apache.org/r/36048/#comment148324 // Forward declaration. src/tests/authorization_tests.cpp (line 48) https://reviews.apache.org/r/36048/#comment148329 Please stick to Owned as before. Less probability of mistakes (even though I don't see any in the present code). - Bernd Mathiske On Aug. 3, 2015, 2:47 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36048/ --- (Updated Aug. 3, 2015, 2:47 a.m.) Review request for mesos, Adam B, Alexander Rukletsov, Bernd Mathiske, Kapil Arya, Jan Schlicht, and Till Toenshoff. Bugs: MESOS-2946 https://issues.apache.org/jira/browse/MESOS-2946 Repository: mesos Description --- Splits and updates the original declaration of the `Authorizer` into its interface and a default implementation, the `LocalAuthorizer`. Following the pattern of the modularized `Authenticator`, it generates a default constructor which is required when writing a `TYPED_TEST` in a follow up patch. Additionally, an initialize method has been added, needed for passing in the current ACL definitions as provided via flags. Other changes are just updates to allow for compilation. Diffs - include/mesos/authorizer/authorizer.hpp PRE-CREATION include/mesos/authorizer/authorizer.proto PRE-CREATION include/mesos/mesos.proto cb24125a3f05e0d38fb22e481a15ceb21f882d27 include/mesos/type_utils.hpp 86b37ca1f63e4687af4e86731dceeb1c9c219c5e src/Makefile.am e2cbd1524f25b3e3a9419af6bdf8cc6e5022a784 src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/examples/persistent_volume_framework.cpp c6d6ed337bfca91dc146cb31298cabebdbb13509 src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f src/master/http.cpp 6f5ca02c52462495b84e77525a6c88299746ece2 src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1 src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0 src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b src/tests/mesos.hpp 23d9841c7cfb16abef89a53d29a256a2c5e94b52 src/tests/mesos.cpp f09ef0f99573716de8905f49dcc0c9df20e31ea9 Diff: https://reviews.apache.org/r/36048/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 36048: Updated authorizer to allow for modularized implementations.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36048/#review93951
---
I have a stupid question. Why shouldn't the authorizer.{hpp,proto} file be
placed inside `include/mesos/master/` instead since the authorizer is only for
the master. Does it make sense to do that? If it does, then should we also move
src/authorize inside src/master?
- Kapil Arya
On Aug. 3, 2015, 5:47 a.m., Alexander Rojas wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36048/
---
(Updated Aug. 3, 2015, 5:47 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Bernd Mathiske, Kapil
Arya, Jan Schlicht, and Till Toenshoff.
Bugs: MESOS-2946
https://issues.apache.org/jira/browse/MESOS-2946
Repository: mesos
Description
---
Splits and updates the original declaration of the `Authorizer` into its
interface and a default implementation, the `LocalAuthorizer`.
Following the pattern of the modularized `Authenticator`, it generates a
default constructor which is required when writing a `TYPED_TEST` in
a follow up patch. Additionally, an initialize method has been added, needed
for passing in the current ACL definitions as provided via
flags.
Other changes are just updates to allow for compilation.
Diffs
-
include/mesos/authorizer/authorizer.hpp PRE-CREATION
include/mesos/authorizer/authorizer.proto PRE-CREATION
include/mesos/mesos.proto cb24125a3f05e0d38fb22e481a15ceb21f882d27
include/mesos/type_utils.hpp 86b37ca1f63e4687af4e86731dceeb1c9c219c5e
src/Makefile.am e2cbd1524f25b3e3a9419af6bdf8cc6e5022a784
src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f
src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc
src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098
src/examples/persistent_volume_framework.cpp
c6d6ed337bfca91dc146cb31298cabebdbb13509
src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff
src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f
src/master/http.cpp 6f5ca02c52462495b84e77525a6c88299746ece2
src/master/main.cpp fd4de4d0d9c3e9617408022d10b5e161bdc911e1
src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec
src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87
src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b
src/tests/mesos.hpp 23d9841c7cfb16abef89a53d29a256a2c5e94b52
src/tests/mesos.cpp f09ef0f99573716de8905f49dcc0c9df20e31ea9
Diff: https://reviews.apache.org/r/36048/diff/
Testing
---
make check
Thanks,
Alexander Rojas
Re: Review Request 37023: Add an endpoint that exposes component flags.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37023/#review93952
---
Ship it!
Nice work! Just a couple of small fixes, notably we're trying to avoid the
.json extension going forward.
src/master/http.cpp (line 329)
https://reviews.apache.org/r/37023/#comment148384
Can you remove start here and for the slave?
src/master/http.cpp (line 546)
https://reviews.apache.org/r/37023/#comment148390
Could you fix this in a separate patch?
src/master/http.cpp (line 958)
https://reviews.apache.org/r/37023/#comment148389
Could you fix this in a separate patch?
src/master/master.cpp (line 764)
https://reviews.apache.org/r/37023/#comment148387
Let's just do /flags, since we're trying to avoid the .json extension.
src/master/master.cpp (line 767)
https://reviews.apache.org/r/37023/#comment148386
Mind logging this, like we do for state.json?
src/slave/http.cpp (line 187)
https://reviews.apache.org/r/37023/#comment148392
s/start//
src/slave/slave.cpp (line 497)
https://reviews.apache.org/r/37023/#comment148388
Let's just do /flags, since we're trying to avoid the .json extension.
src/slave/slave.cpp (line 500)
https://reviews.apache.org/r/37023/#comment148385
Mind logging this, like we do for state.json?
- Ben Mahler
On Aug. 2, 2015, 9:28 a.m., haosdent huang wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37023/
---
(Updated Aug. 2, 2015, 9:28 a.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-3104
https://issues.apache.org/jira/browse/MESOS-3104
Repository: mesos
Description
---
Add an endpoint that exposes component flags.
Diffs
-
src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f
src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc
src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544
src/slave/http.cpp b0fe5f520dfca156548ba8c436d42fc432223f3d
src/slave/slave.hpp 41d09497be313819a9c78361b8595f6f26dc8460
src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1
Diff: https://reviews.apache.org/r/37023/diff/
Testing
---
manual test wich mesos-local.
```
$ curl http://localhost:5050/master/flags.json 2/dev/null|jq .
{
flags: {
allocation_interval: 1secs,
allocator: HierarchicalDRF,
authenticate: false,
authenticate_slaves: false,
authenticators: crammd5,
framework_sorter: drf,
help: true,
initialize_driver_logging: true,
log_auto_initialize: true,
logbufsecs: 0,
logging_level: INFO,
max_slave_ping_timeouts: 5,
quiet: false,
recovery_slave_removal_limit: 100%,
registry: replicated_log,
registry_fetch_timeout: 1mins,
registry_store_timeout: 5secs,
registry_strict: false,
root_submissions: true,
slave_ping_timeout: 15secs,
slave_reregister_timeout: 10mins,
user_sorter: drf,
version: false,
webui_dir: /home/haosdent/mesos/build/../src/webui,
work_dir: /tmp/mesos,
zk_session_timeout: 10secs
}
}
```
```
$ curl http://localhost:5050/slave(1)/flags.json 2/dev/null|jq .
{
flags: {
authenticatee: crammd5,
cgroups_cpu_enable_pids_and_tids_count: false,
cgroups_enable_cfs: false,
cgroups_hierarchy: /sys/fs/cgroup,
cgroups_limit_swap: false,
cgroups_root: mesos,
container_disk_watch_interval: 15secs,
containerizers: mesos,
default_role: *,
disk_watch_interval: 1mins,
docker: docker,
docker_kill_orphans: true,
docker_remove_delay: 6hrs,
docker_socket: /var/run/docker.sock,
docker_stop_timeout: 0ns,
enforce_container_disk_quota: false,
executor_registration_timeout: 1mins,
executor_shutdown_grace_period: 5secs,
fetcher_cache_dir: /tmp/mesos/fetch,
fetcher_cache_size: 2GB,
frameworks_home: ,
gc_delay: 1weeks,
gc_disk_headroom: 0.1,
hadoop_home: ,
help: false,
initialize_driver_logging: true,
isolation: posix/cpu,posix/mem,
launcher_dir: /home/haosdent/mesos/build/src,
logbufsecs: 0,
logging_level: INFO,
oversubscribed_resources_interval: 15secs,
perf_duration: 10secs,
perf_interval: 1mins,
qos_correction_interval_min: 0ns,
quiet: false,
recover: reconnect,
recovery_timeout: 15mins,
registration_backoff_factor: 1secs,
resource_monitoring_interval: 1secs,
revocable_cpu_low_priority: true,
sandbox_directory: /mnt/mesos/sandbox,
strict: true,
switch_user: true,
version: false,
work_dir: /tmp/mesos/0
}
}
```
Thanks,
haosdent huang
Re: Review Request 36402: Adding 'Accept' header in request
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated Aug. 3, 2015, 8:19 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp b8d9300 3rdparty/libprocess/src/http.cpp 4dcbd74 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd5 Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 37007: Pass ExecutorID to task-status label decorator hook.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37007/#review93963 --- include/mesos/hook.hpp (lines 89 - 90) https://reviews.apache.org/r/37007/#comment148416 The executor id should be in the task status (https://github.com/apache/mesos/blob/master/include/mesos/mesos.proto#L973) have you run into it not being available? - Niklas Nielsen On July 31, 2015, 9:24 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37007/ --- (Updated July 31, 2015, 9:24 p.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Niklas Nielsen. Bugs: MESOS-3016 https://issues.apache.org/jira/browse/MESOS-3016 Repository: mesos Description --- Currently, only FrameworkID and TaskID are sent to the hook. Diffs - include/mesos/hook.hpp bb5a635dcf189e1023f1eec66fc06955f816fc0b src/examples/test_hook_module.cpp c664b565bcf18dd2153205990119cc679e4ad6cf src/hook/manager.hpp 8153ce4826f94d5771c93d37c59fdc4991352e66 src/hook/manager.cpp 11e6b0a2c0df1d0d7039aaad94e1c6f0e5cc6bc2 src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1 Diff: https://reviews.apache.org/r/37007/diff/ Testing --- make check. Thanks, Kapil Arya
Review Request 37045: Convert Linux perf sampler to use process:await().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37045/ --- Review request for mesos and Ben Mahler. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Convert Linux perf sampler to use process:await(). Diffs (updated) - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/37045/diff/ Testing (updated) --- sudo make check Thanks, Paul Brett
Re: Review Request 36867: Add labels to FrameworkInfo.
On July 27, 2015, 11:36 p.m., Adam B wrote: Great first patch. Thanks for updating FrameworkInfo on reregistration with the master too! A handful of nits in my first pass. I'll take another look once you've simplified the tests with Kapil's suggestions. Any updates here? :) - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36867/#review93242 --- On July 27, 2015, 6:25 p.m., Neil Conway wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36867/ --- (Updated July 27, 2015, 6:25 p.m.) Review request for mesos, Adam B, Connor Doyle, and Niklas Nielsen. Bugs: MESOS-2841 https://issues.apache.org/jira/browse/MESOS-2841 Repository: mesos Description --- This is intended to support frameworks that want to offer capabilities to the rest of the cluster (e.g., storage or some arbitrary third-party service). We want processes running in the cluster to be able to discover these capabilities; however, we don't want to commit to a fixed set of capabilities or how those capabilities should be represented. Hence, this commit represents this information using freeform key-value pairs, similar to the labels mechanism already in use elsewhere. Jira: MESOS-2841 Diffs - include/mesos/mesos.proto a6748d1cd82238f005c6a49c70d22d095462f1ba src/master/http.cpp 3a1598fad4db03e5f62fd4a6bd26b2bedeee4070 src/master/master.hpp 2c924addfb4c52d3048ee6ded13ce638145cc93f src/tests/fault_tolerance_tests.cpp 7b977f5e8195d9f42b21f36eb36fb156471caa20 src/tests/master_tests.cpp 05c148ee1660b86428afe4eda718b17052743a8c Diff: https://reviews.apache.org/r/36867/diff/ Testing --- make check Thanks, Neil Conway
Review Request 37046: Merged registerFramework() and reregisterFramework().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37046/ --- Review request for mesos, Anand Mazumdar and Ben Mahler. Bugs: MESOS-3182 https://issues.apache.org/jira/browse/MESOS-3182 Repository: mesos Description --- Merged registerFramework() and reregisterFramework() (and their corresponding continuations) into subscribe() (and _subscribe()). Diffs - src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 Diff: https://reviews.apache.org/r/37046/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 37024: Add an endpoint that exposes component version.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37024/#review93947
---
Thanks! Let's include the git / build information as well.
Another question, how are you planning to add this to the scheduler driver and
executor driver? In these cases, it is likely better to create a 'Version'
Process with process id version that routes an endpoint at / (so just
/version should route to this). If we go with this approach, we just have to
start a 'Version' Process from the master/slave entrypoints and the drivers'
initialization.
src/master/http.cpp (lines 526 - 542)
https://reviews.apache.org/r/37024/#comment148381
Could you include these fields as well? They capture version / build
information. Once you do, perhaps it is worth adding a '`JSON::Object
version()`' helper in common/http.hpp.
- Ben Mahler
On Aug. 2, 2015, 10:16 a.m., haosdent huang wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37024/
---
(Updated Aug. 2, 2015, 10:16 a.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-1841
https://issues.apache.org/jira/browse/MESOS-1841
Repository: mesos
Description
---
Add an endpoint that exposes component version.
Diffs
-
src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f
src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc
src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544
src/slave/http.cpp b0fe5f520dfca156548ba8c436d42fc432223f3d
src/slave/slave.hpp 41d09497be313819a9c78361b8595f6f26dc8460
src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1
Diff: https://reviews.apache.org/r/37024/diff/
Testing
---
Manual test result:
```
$ curl http://localhost:5050/slave(1)/version.json 2/dev/null|jq .
{
version: 0.24.0
}
```
```
$ curl http://localhost:5050/master/version.json 2/dev/null|jq .
{
version: 0.24.0
}
```
Thanks,
haosdent huang
Re: Review Request 36987: Extend 'getFormValue' - 'getFormValues' to parse input uniformly.
On Aug. 3, 2015, 6:43 p.m., Ben Mahler wrote: src/master/http.cpp, lines 361-362 https://reviews.apache.org/r/36987/diff/2/?file=1026437#file1026437line361 Do we even want to keep this? Note that the 'observe' has never been used. I've found this to be a strange helper in the first place, how would we explain what this does in a comment? Michael Park wrote: Note that the 'observe' has never been used. What do you mean? It looks like `/observe` is an exposed endpoint that calls `observe`? I've found this to be a strange helper in the first place, how would we explain what this does in a comment? I should've accompanied it with a comment. My view of its functionality is something like: ``` Takes an HTTP request and the expected list of keys, tries to return the map of expected keys to their corresponding values. It returns an Error if we failed to parse the request, if an expected key was not provided, or if the corresponding value is an empty string. Otherwise, the resulting hashmap contains the mapping from expected keys to provided values. ``` I think it's a useful helper function for capturing the above functionality as well as to facilitate providing consistent error messages. (e.g. for `/observe` we returned Missing value for 'monitor'., whereas for `/teardown` we returned Missing 'frameworkId' query parameter) Ben Mahler wrote: See the TODO in `observe`, it doesn't do anything yet :) It just seems unfortunate that without such a big comment, one can't easily intuit what this helper does (i.e. not very readable). What are form values anyway? :) How are optional query parameters dealt with? Also the empty string error case seems pretty implicit? See the TODO in observe, it doesn't do anything yet :) I see. What are form values anyway? :) I inherited this from the `getFormValue` function which existed prior to this. My guess was that it refers to HTML Forms. How are optional query parameters dealt with? This is a good point, I think perhaps what we ideally want is something closer to a commang-line parser, where we can specify required/optional keys? I still think this is a good start to that level of abstraction though. MVP so-to-speak I guess? Also the empty string error case seems pretty implicit? I agree, also inherited from the `getFormValue`. Would be happy to remove this implicit behavior. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36987/#review93942 --- On July 31, 2015, 9:56 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36987/ --- (Updated July 31, 2015, 9:56 p.m.) Review request for mesos and Jie Yu. Repository: mesos Description --- See summary. Diffs - src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f src/tests/repair_tests.cpp 4d513ed9e18ebf04a5e5aadb022b064479739e66 Diff: https://reviews.apache.org/r/36987/diff/ Testing --- `make check` Thanks, Michael Park
Re: Review Request 34427: AppC provisioner backend using bind mounts.
On May 21, 2015, 12:29 p.m., Timothy Chen wrote: src/slave/containerizer/provisioners/appc/bind_backend.hpp, line 70 https://reviews.apache.org/r/34427/diff/1/?file=964174#file964174line70 Should we make rootfs a constant somewhere? Yeah, I think there should be a paths.hpp utility to do all path-related work. - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34427/#review84771 --- On June 22, 2015, 9:53 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34427/ --- (Updated June 22, 2015, 9:53 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Note: This is a specialized backend; see notes in bind_backend.hpp. Reproduced here for your convenience: This is a specialized backend that may be useful for deployments using large (multi-GB) single-layer images *and* where more recent kernel features such as overlayfs are not available. For small images (10's to 100's of MB) the Copy backend may be sufficient. 1) It supports only a single layer. Multi-layer images will fail to provision and the container will fail to launch! 2) The filesystem is read-only because all containers using this image share the source. Select writable areas can be achieved by mounting read-write volumes to places like /tmp, /var/tmp, /home, etc. using the ContainerInfo. These can be relative to the executor work directory. 3) It relies on the image persisting in the store. 4) It's fast because the bind mount requires (nearly) zero IO. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION src/slave/containerizer/provisioners/appc/bind_backend.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34427/diff/ Testing --- manual testing of a single layer image with RW relative bind mount for /tmp. Thanks, Ian Downes
