Re: Review Request 32845: Renamed UNREGISTER call to UNSUBSCRIBE.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32845/#review80870 --- include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32845/#comment131014 Is UNSUBSCRIBE the right name for this? It seems like unsubscribe would stop the subscription for events, rather than shutdown all the tasks and remove the framework. That seems prone to a bit of confusion and mistakes? For example, would an operator remove a framework with an /unsubscribe endpoint? Hm.. it seems like SHUTDOWN would have been the best names for this? Should we re-use KILL for executors, but use SHUTDOWN for frameworks? - Ben Mahler On April 20, 2015, 8:04 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32845/ --- (Updated April 20, 2015, 8:04 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Renamed UNREGISTER call to UNSUBSCRIBE for consistency. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32845/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32502: Updated RECONCILE call to always specifiy slave id.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32502/#review80827 --- Ship it! include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32502/#comment130934 This still refers to 'statuses'? src/master/master.cpp https://reviews.apache.org/r/32502/#comment130936 How about some newlines here? You could set the slave id last since it requires an 'if'. src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32502/#comment130938 Maybe we should test without this set? - Ben Mahler On April 20, 2015, 7:59 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32502/ --- (Updated April 20, 2015, 7:59 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Having a slave id will help us do better reconciliation. So added it to the 'Reconcile' call. Also updated the master to receive the Reconcile call directly. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp c10e7c08c191acef9d31d98018a47a2a952a4dfc src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32502/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32844: Added SUBSCRIBE call and SUBSCRIBED event.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32844/#review80855 --- Looks good. I asked below: why did the original code process (re-)registration messages when we haven't detected a master? include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32844/#comment130970 Might help to have a hint pointing to the framework id semantics: ``` SUBSCRIBE = 1; // See 'framework_info' below. ``` src/examples/low_level_scheduler_libprocess.cpp https://reviews.apache.org/r/32844/#comment130971 Maybe just Subscribed with ID X? src/examples/low_level_scheduler_pthread.cpp https://reviews.apache.org/r/32844/#comment130973 Ditto here. src/master/master.cpp https://reviews.apache.org/r/32844/#comment130974 TODO for implementing this? I assume you held off because of the non-trivial streaming related changes needed? src/scheduler/scheduler.cpp https://reviews.apache.org/r/32844/#comment130975 This check considers set to `` as unset, but the check above does not? (Where we check for missing `FrameworkInfo.id` in the `Call`) src/scheduler/scheduler.cpp https://reviews.apache.org/r/32844/#comment130976 Hm, should we re-phrase this? I don't know what it means :) src/scheduler/scheduler.cpp https://reviews.apache.org/r/32844/#comment130979 This isn't yours, but is this right? We're looking at SUBSCRIBED events when the master is `None`? - Ben Mahler On April 20, 2015, 8:03 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32844/ --- (Updated April 20, 2015, 8:03 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Instead of REGISTER and REREGISTER we now just have SUBSCRIBE. Similarly, instead of REGISTERED and REREGISTERED there is only SUBSCRIBED. This will simplify a scheduler's registration semantics. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp e30b951eda2b3b0d5b2a80716f0b32c6bbe041bc src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32844/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 31665: Changed '(used|offered)Resources' to 'total(Used|Offered)Resources' and added 'hashmapSlaveID, Resources (used|offered)Resources' to 'master::Framework'
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31665/#review80528 --- Ship it! - Ben Mahler On April 17, 2015, 5:31 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31665/ --- (Updated April 17, 2015, 5:31 a.m.) Review request for mesos, Alexander Rukletsov and Ben Mahler. Bugs: MESOS-2373 https://issues.apache.org/jira/browse/MESOS-2373 Repository: mesos Description --- `master::Framework` holds 2 member variables of type `Resources`: `usedResources` and `offeredResources`. Both of these are aggregates of resources from multiple slaves. We add the `hashmapSlaveID, Resources` versions since we can lose non-scalar resources by summing them up across multiple slaves. For further details refer to [MESOS-2373](https://issues.apache.org/jira/browse/MESOS-2373). In [r31666](https://reviews.apache.org/r/31666/), we report `usedResourcesBySlaveId` rather than `usedResources` when adding the framework to the allocator. We don't actually use `offeredResourcesBySlaveId` currently, but it should be kept for symmetry. There will be a ticket to expose `usedResourcesBySlaveId` as well as `offeredResourcesBySlaveId` via the HTTP endpoint. Diffs - src/master/http.cpp f2b123d7f84eafc792849ad4630c2e3b63be000b src/master/master.hpp 4a1df58e6756bff7d5f292c0542023a9215e33d8 src/master/master.cpp 02f35ac2d98d288bf9d2b9195770b839bbfe6d7a Diff: https://reviews.apache.org/r/31665/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 27113: Libprocess benchmark cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review80540 --- Ship it! Wow, nice work, this is a lot cleaner! I left some comments but since they are pretty minor, I've gone ahead and taken care of them to get this committed for you. For the dependent change of disabling local messages, I've left it as a TODO here so we can get these great cleanups in. Let's follow up with your dependent patch to add the ability for the tests to run both the local and remote code paths, sound good? :) 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130567 Looks like we're missing some includes here? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130574 Weird wrapping here? You could make this comment more succinct here if you omit the request parameters. We probably don't need a /help for now. 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130572 Maybe a newline in between these? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130563 It seems a bit strange to have a `running` that isn't set to false? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130570 Since we're capturing the duration for the run, how about just calling it 'duration'? Finished makes it clear that it's a signal for finishing but that seems to express how it's used as opposed to what it is. 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130575 It's probably time we give this a more meaningful name than Test. :) 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130561 No need for `` here. 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130568 How about: ``` // Start the ping / pongs! const string query = strings::join( , server= + stringify(serverPid), requests= + stringify(numRequests), concurrency= + stringify(concurrency), messageSize= + stringify(messageSize)); ``` 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130559 Can we use std::array in the future? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130543 Hm.. we call this 'requests' but we store 'Responses'? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130548 It seems like the iterate and wait ... aspect of this comment re-iterates what the code says? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130556 A 'request' is actually a 'Response'? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130553 Printing the total rpc throughput like this seems prone to error. For example, if each client ran __serially__ at throughput X, we would print X*N without realizing that each client's throughput was not overlapping. Could we just print the throughput for each individual client for now? For server throughput, we can track that more precisely in the server itself, yes? For now, I'd propose just tracking the total time in the test and estimating total throughput based on that. Also, why not use cout like we did elsewhere? - Ben Mahler On April 17, 2015, 5:28 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/ --- (Updated April 17, 2015, 5:28 p.m.) Review request for mesos, Ben Mahler, Cody Maloney, Joerg Schad, and Michael Park. Bugs: MESOS-1980 https://issues.apache.org/jira/browse/MESOS-1980 Repository: mesos Description --- Clean up Libprocess for readability / extensibility. Diffs - 3rdparty/libprocess/src/tests/benchmarks.cpp a927e4ecd8c8955cd9f85e716173a73a9a21c6cd Diff: https://reviews.apache.org/r/27113/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 33154: Added reason metrics for slave removals.
On April 16, 2015, 4 p.m., Vinod Kone wrote: src/master/master.cpp, line 4465 https://reviews.apache.org/r/33154/diff/1/?file=926687#file926687line4465 Is this optional because there are other removals that we don't have metrics for? the ones in exited() and reregisteredSlave()? can we have metrics for them too while we are at it? I can add one for when the a new slave registers and replaces the old slave. For exited(), I didn't add a metric since we are removing checkpointing. It doesn't look like the flag is even available anymore? Looks like some further cleanup is required (remove the field in the message, and the comment / logic in `Master::exited`). - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33154/#review80339 --- On April 14, 2015, 1:46 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33154/ --- (Updated April 14, 2015, 1:46 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2485 https://issues.apache.org/jira/browse/MESOS-2485 Repository: mesos Description --- See [MESOS-2485](https://issues.apache.org/jira/browse/MESOS-2485). Diffs - include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 src/master/master.hpp 6141917644b84edfed9836fa0a005d55a36880e3 src/master/master.cpp 44b0a0147f5354824d86332a67b30018634c9a36 src/master/metrics.hpp 52a83289cfe7e6b6fd8d5bff0774ebe5ce51d0ed src/master/metrics.cpp 14486bf7130250f561c9fb7a43c95f3fc1e76a4b Diff: https://reviews.apache.org/r/33154/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 33154: Added reason metrics for slave removals.
On April 16, 2015, 4 p.m., Vinod Kone wrote: src/master/master.cpp, line 4465 https://reviews.apache.org/r/33154/diff/1/?file=926687#file926687line4465 Is this optional because there are other removals that we don't have metrics for? the ones in exited() and reregisteredSlave()? can we have metrics for them too while we are at it? Ben Mahler wrote: I can add one for when the a new slave registers and replaces the old slave. For exited(), I didn't add a metric since we are removing checkpointing. It doesn't look like the flag is even available anymore? Looks like some further cleanup is required (remove the field in the message, and the comment / logic in `Master::exited`). Sorry, s/removing checkpointing/removing the ability to disable checkpointing support/ - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33154/#review80339 --- On April 14, 2015, 1:46 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33154/ --- (Updated April 14, 2015, 1:46 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2485 https://issues.apache.org/jira/browse/MESOS-2485 Repository: mesos Description --- See [MESOS-2485](https://issues.apache.org/jira/browse/MESOS-2485). Diffs - include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 src/master/master.hpp 6141917644b84edfed9836fa0a005d55a36880e3 src/master/master.cpp 44b0a0147f5354824d86332a67b30018634c9a36 src/master/metrics.hpp 52a83289cfe7e6b6fd8d5bff0774ebe5ce51d0ed src/master/metrics.cpp 14486bf7130250f561c9fb7a43c95f3fc1e76a4b Diff: https://reviews.apache.org/r/33154/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 31665: Added 'hashmapSlaveID, Resources (used|offered)ResourcesBySlaveId' to master::Framework.
On April 15, 2015, 7:20 p.m., Ben Mahler wrote: src/master/master.hpp, lines 1138-1143 https://reviews.apache.org/r/31665/diff/5/?file=927382#file927382line1138 Thanks for this NOTE! We might want to take this opportunity to articulate why we can safely keep the totals. Or, should we strip non-scalars and store a 'totalUsedScalarResources'? Michael Park wrote: We can't do the former since we can't safely keep the totals. We might want to do something like the latter but I think we may break some people that way, and therefore should be done as a subsequent task. I've created [MESOS-2623](https://issues.apache.org/jira/browse/MESOS-2623) to track this. We can't do the former since we can't safely keep the totals. Ah ok great, can we articulate why for posterity? It's not immediately obvious to me. As in, is there a bug due to this? Or is it just that the non-scalar information is not correct, and there's not yet a bug due to this since we only expose scalars? Something else? On April 15, 2015, 7:20 p.m., Ben Mahler wrote: src/master/master.hpp, lines 1145-1147 https://reviews.apache.org/r/31665/diff/5/?file=927382#file927382line1145 How about s/usedResources/totalUsedResources/ and s/usedResourcesBySlaveId/usedResources/ ? This might also make the total referred to in your note a bit clearer. Ditto for offered. Michael Park wrote: Yeah, I considered `totalUsedResources` and `usedResources` and I liked those. I went with `usedResources` and `usedResourcesBySlaveId` because I thought exposing `totalUsedResources` via `used_resources`, then having to expose `usedResources` via some other key like `used_resources_by_slave_id` would create confusion later on. This way `usedResources` maps to `used_resources` and `usedResourcesBySlaveId` can map to `used_resources_by_slave_id`. Unless we decide that we're never going to expose `usedResourcesBySlaveId`, in which case we'll have `totalUsedResources` mapped to `used_resources` and that'll be it. What do you think? That makes sense, although, since we both had the same initial intuition, others may as well. I think it's ok not to allow the JSON format to affect our variable naming, since we want to make sure that the code is as readable / understandable as possible. I like the clarified total and used naming scheme you considered initially because it allows us to make it clear that used resources have to be namespaced by slaves to be correctly represented, whereas the total only makes sense for scalars (we can have a note about that). Someone is more likely to ponder this, especially if it's called `'totalUsedScalarResources'`. Feel free to punt on stripping non-scalars if you want to tackle it later! - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31665/#review80252 --- On April 15, 2015, 10:45 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31665/ --- (Updated April 15, 2015, 10:45 p.m.) Review request for mesos, Alexander Rukletsov and Ben Mahler. Bugs: MESOS-2373 https://issues.apache.org/jira/browse/MESOS-2373 Repository: mesos Description --- `master::Framework` holds 2 member variables of type `Resources`: `usedResources` and `offeredResources`. Both of these are aggregates of resources from multiple slaves. We add the `hashmapSlaveID, Resources` versions since we can lose non-scalar resources by summing them up across multiple slaves. For further details refer to [MESOS-2373](https://issues.apache.org/jira/browse/MESOS-2373). In [r31666](https://reviews.apache.org/r/31666/), we report `usedResourcesBySlaveId` rather than `usedResources` when adding the framework to the allocator. We don't actually use `offeredResourcesBySlaveId` currently, but it should be kept for symmetry. There will be a ticket to expose `usedResourcesBySlaveId` as well as `offeredResourcesBySlaveId` via the HTTP endpoint. Diffs - src/master/master.hpp 6141917644b84edfed9836fa0a005d55a36880e3 Diff: https://reviews.apache.org/r/31665/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 31665: Added 'hashmapSlaveID, Resources (used|offered)ResourcesBySlaveId' to master::Framework.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31665/#review80370 --- src/master/master.hpp https://reviews.apache.org/r/31665/#comment130237 For example, could we document in NOTE on these total variables that subtraction is safe even though non-scalars lose information? Specifically, is my understanding correct here: (1) For overlapping set items / ranges across slaves, these will get added N times but only represented 1 time in the Resources. (2) So, when an initial subtraction occurs (N-1), the resource is no longer represented. We don't expose non-scalars for now, so this is safe for now. (3) When any further subtractions occur (N-(1+M)), the Resources simply ignores the subtraction since there's nothing to remove, so this is safe for now. - Ben Mahler On April 15, 2015, 10:45 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31665/ --- (Updated April 15, 2015, 10:45 p.m.) Review request for mesos, Alexander Rukletsov and Ben Mahler. Bugs: MESOS-2373 https://issues.apache.org/jira/browse/MESOS-2373 Repository: mesos Description --- `master::Framework` holds 2 member variables of type `Resources`: `usedResources` and `offeredResources`. Both of these are aggregates of resources from multiple slaves. We add the `hashmapSlaveID, Resources` versions since we can lose non-scalar resources by summing them up across multiple slaves. For further details refer to [MESOS-2373](https://issues.apache.org/jira/browse/MESOS-2373). In [r31666](https://reviews.apache.org/r/31666/), we report `usedResourcesBySlaveId` rather than `usedResources` when adding the framework to the allocator. We don't actually use `offeredResourcesBySlaveId` currently, but it should be kept for symmetry. There will be a ticket to expose `usedResourcesBySlaveId` as well as `offeredResourcesBySlaveId` via the HTTP endpoint. Diffs - src/master/master.hpp 6141917644b84edfed9836fa0a005d55a36880e3 Diff: https://reviews.apache.org/r/31665/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 31666: Sent 'framework-usedResourcesBySlaveId' to allocator instead for 'addFramework'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31666/#review80257 --- Ship it! src/master/allocator/mesos/hierarchical.hpp https://reviews.apache.org/r/31666/#comment130059 Should there be a TODO here related to what needs to be done? - Ben Mahler On April 15, 2015, 3:54 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31666/ --- (Updated April 15, 2015, 3:54 p.m.) Review request for mesos, Alexander Rukletsov and Ben Mahler. Bugs: MESOS-2373 https://issues.apache.org/jira/browse/MESOS-2373 Repository: mesos Description --- Piped the master through by `s/framework-usedResources/framework-usedResourcesBySlaveId/` Lots of `s/Resources()/hashmapSlaveID, Resources()/` in `src/tests/hierarchical_allocator_tests.cpp`. Diffs - src/master/allocator/allocator.hpp 91f80501aa9bc733fd53f9cb1ac0f15949a74964 src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/allocator/mesos/hierarchical.hpp 9f9a631ee52a3ab194e678f9be139e8dabc7f3db src/master/master.cpp 44b0a0147f5354824d86332a67b30018634c9a36 src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/mesos.hpp 42e42ac425a448fcc5e93db1cef1112cbf5e67c4 Diff: https://reviews.apache.org/r/31666/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 31665: Added 'hashmapSlaveID, Resources (used|offered)ResourcesBySlaveId' to master::Framework.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31665/#review80252 --- There are no erase calls on these new maps? When are you removing the slave? src/master/master.hpp https://reviews.apache.org/r/31665/#comment130057 Thanks for this NOTE! We might want to take this opportunity to articulate why we can safely keep the totals. Or, should we strip non-scalars and store a 'totalUsedScalarResources'? src/master/master.hpp https://reviews.apache.org/r/31665/#comment130056 How about s/usedResources/totalUsedResources/ and s/usedResourcesBySlaveId/usedResources/ ? This might also make the total referred to in your note a bit clearer. Ditto for offered. - Ben Mahler On April 14, 2015, 7:40 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31665/ --- (Updated April 14, 2015, 7:40 p.m.) Review request for mesos, Alexander Rukletsov and Ben Mahler. Bugs: MESOS-2373 https://issues.apache.org/jira/browse/MESOS-2373 Repository: mesos Description --- `master::Framework` holds 2 member variables of type `Resources`: `usedResources` and `offeredResources`. Both of these are aggregates of resources from multiple slaves. We add the `hashmapSlaveID, Resources` versions since we can lose non-scalar resources by summing them up across multiple slaves. For further details refer to [MESOS-2373](https://issues.apache.org/jira/browse/MESOS-2373). In [r31666](https://reviews.apache.org/r/31666/), we report `usedResourcesBySlaveId` rather than `usedResources` when adding the framework to the allocator. We don't actually use `offeredResourcesBySlaveId` currently, but it should be kept for symmetry. There will be a ticket to expose `usedResourcesBySlaveId` as well as `offeredResourcesBySlaveId` via the HTTP endpoint. Diffs - src/master/master.hpp 6141917644b84edfed9836fa0a005d55a36880e3 Diff: https://reviews.apache.org/r/31665/diff/ Testing --- make check Thanks, Michael Park
Re: Review Request 27113: Libprocess benchmark cleanup
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27113/#review80101
---
Wow, the client and server parts of this are looking a lot clearer, thanks
Joris!
Let's focus on the test body next, it was difficult for me to grasp it fully, I
left a comment below :)
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129861
How about 'concurrency' or 'concurrentRequests' instead of 'queueDepth'?
Which queue is this variable referring to?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129863
This seems to suggest support for multiple runs of the client. However,
if a /run request arrives while the previous run is in progress, it looks like
this will behave strangely. Did you want to deny requests while a run is in
progress, or chain the runs after one another?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129864
Why do totalRequests and the concurrency factor need to be reset here?
Looks like these are required.
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129871
How about:
```
hashmapstring, Optionstring parameters {
{server, request.query.get(server)},
{messageSize, request.query.get(messageSize)},
{totalRequests, request.query.get(totalRequests)},
{concurrentRequests, request.query.get(concurrentRequests)},
};
// Ensure all parameters were provided.
foreachpair (const string parameter, const Optionstring value,
parameters) {
if (value.isNone()) {
return http::BadRequest(Missing ' + parameter + ' parameter);
}
}
server = UPID(parameters[server].get());
TryBytes bytes = Bytes::parse(parameters[messageSize].get());
if (bytes.isError()) {
return http::BadRequest(Invalid 'messageSize': + bytes.error());
}
messageSize = bytes.get();
Trysize_t numify = numifysize_t(parameters[totalRequests].get());
if (numify.isError()) {
return http::BadRequest(Invalid 'totalRequests': + numify.error());
}
totalRequests = numify.get();
numify = numifysize_t(parameters[concurrentRequests].get());
if (numify.isError()) {
return http::BadRequest(Invalid 'concurrentRequests': +
numify.error());
}
concurrentRequests = numify.get();
```
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129865
How about using a `Bytes` for this?
```
TryBytes messageSize = Bytes::parse(value.get());
if (messageSize.isError()) {
}
message = string(messageSize.get().bytes(), '1');
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129866
Can you include the error message from the Try here and elsewhere?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129873
Why do you need to reject this? If the number of concurrent requests is
larger than the total number of requests, that seems well defined
(concurrentRequests == totalRequests). Any reason not to just do the right
thing for what was requested?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129875
'concurrency' or 'concurrentRequests' might make this clearer
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129877
Comment seems unnecessary?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129881
Could you do a self review of this test body, from the perspective of
someone approaching this code for the first time?
I had a hard time understanding this, at a basic level what is the overall
structure of the forking here? It's also a bit tricky to figure out the lower
level details, like what the `requestPipes` and `resultPipes` are for, and why
they're needed. What the vectors of pipes are used for, why they're needed. Etc.
In the process of being able to articulate what this does, we might figure
out how we can make it more easily understandable :)
- Ben Mahler
On April 14, 2015, 7:46 p.m., Joris Van Remoortere wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27113/
---
(Updated April 14, 2015, 7:46 p.m.)
Review request for mesos, Ben Mahler, Cody Maloney, Joerg Schad, and Michael
Park.
Bugs: MESOS-1980
https://issues.apache.org/jira/browse/MESOS-1980
Repository: mesos
Description
---
Clean up Libprocess for readability / extensibility
Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32585/#review79904 --- src/master/master.hpp https://reviews.apache.org/r/32585/#comment129498 Did you look back at https://reviews.apache.org/r/19176/ when doing this? What was the motivation for making this function instead of a field? Either way (1) the 'id' cannot be changed, and (2) there are two ways to access the ID: id() and info.id(). Is there any benefit I'm missing? Just seems like unnecessary churn in the code to me. Note that this change means the `Slave` and `Framework` structs now store their ID's differently as well, which is unfortunate. - Ben Mahler On April 7, 2015, 4:59 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32585/ --- (Updated April 7, 2015, 4:59 p.m.) Review request for mesos, Adam B and Niklas Nielsen. Bugs: MESOS-2557 https://issues.apache.org/jira/browse/MESOS-2557 Repository: mesos Description --- Framework.id() extracts and returns FrameworkID from Framework.info. Diffs - src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 Diff: https://reviews.apache.org/r/32585/diff/ Testing --- make check TODO: Test for upgrade path. Thanks, Kapil Arya
Review Request 33153: Moved a partition test into partition_tests.cpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33153/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- See summary. Diffs - src/tests/fault_tolerance_tests.cpp a637c32f004638a110390b22cf5b626e904097cf src/tests/partition_tests.cpp 1018e479a77a6b533f2dd392fedbdccb80e3ac1f Diff: https://reviews.apache.org/r/33153/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 33155: Added commented-out tests for slave removal metrics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33155/ --- Review request for mesos and Vinod Kone. Bugs: MESOS-2485 https://issues.apache.org/jira/browse/MESOS-2485 Repository: mesos Description --- I updated the existing tests to validate the metrics. However, it blocks the tests due to the slave metrics never getting satisfied. Added a TODO. Diffs - src/tests/master_tests.cpp 32b1e9bb58d8046e5363fafe2ab8f662b6c9a666 src/tests/partition_tests.cpp 1018e479a77a6b533f2dd392fedbdccb80e3ac1f src/tests/slave_tests.cpp b826000e0a4221690f956ea51f49ad4c99d5e188 Diff: https://reviews.apache.org/r/33155/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 33154: Added reason metrics for slave removals.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33154/ --- Review request for mesos and Vinod Kone. Bugs: MESOS-2485 https://issues.apache.org/jira/browse/MESOS-2485 Repository: mesos Description --- See [MESOS-2485](https://issues.apache.org/jira/browse/MESOS-2485). Diffs - include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 src/master/master.hpp 6141917644b84edfed9836fa0a005d55a36880e3 src/master/master.cpp 44b0a0147f5354824d86332a67b30018634c9a36 src/master/metrics.hpp 52a83289cfe7e6b6fd8d5bff0774ebe5ce51d0ed src/master/metrics.cpp 14486bf7130250f561c9fb7a43c95f3fc1e76a4b Diff: https://reviews.apache.org/r/33154/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 33152: Moved the slave shutdown test into slave_tests.cpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33152/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- See summary. Diffs - src/tests/fault_tolerance_tests.cpp a637c32f004638a110390b22cf5b626e904097cf src/tests/slave_tests.cpp b826000e0a4221690f956ea51f49ad4c99d5e188 Diff: https://reviews.apache.org/r/33152/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 32463: Improved testing around state.json endpoints.
On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: src/tests/master_tests.cpp, line 2816 https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line2816 Why do you check the type here, but not above for e.g. `DATE`, `TIME`, and `USER`? Ah nice catch, `id` needs it but the ones below don't need it! For DATE, TIME, and USER, we can check equality (note that the `std::string build::DATE` implicitly becomes a `JSON::String`, which is `==` comparable to a `JSON::Value`). However, for `id`, we want to confirm that it's a non-empty `JSON::String`. If you check for ` != value` then this holds for all non-empty strings, but it also holds for JSON::Number, JSON::Object, etc. So I check the type explicitly, make sense? On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: src/tests/master_tests.cpp, line 66 https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line66 I noticed you do drive-by clean-up of some `process::` occurrences. Do you want to go further and remove all of them or prefer creating a newbie JIRA for that? There are e.g. `process::Message` and `Process::Time`. Thanks for noting this, I originally added this to avoid having to add using clauses for a lot of things from process::http. However, we generally are avoiding using namespaces like this so I've reverted back to fine-grained using clauses, and a namespace alias at the top to deal with the `proces::http` problem. On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: src/tests/master_tests.cpp, lines 2847-2849 https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line2847 How about a one-liner here and below: ``` EXPECT_TRUE(state.values[slaves].asJSON::Array().values.empty()); ``` Thanks! On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: src/tests/slave_tests.cpp, line 792 https://reviews.apache.org/r/32463/diff/1/?file=904858#file904858line792 Not yours, but can you do a drive-by fix and remove `process::`? I'd like to avoid the noise in the diff since its in an unrelated test :) On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: src/tests/master_tests.cpp, line 2814 https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line2814 This fails on my Mac. Also, since you're checking start time, you can add one more `EXPECT_GT` check caching time before starting master. It turns out this fails because `Time::create` compensates for the advanced state the clock by adding `clock::advanced`. That has already been added from `Clock::now` in the endpoint and so the times are not equal. On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: src/tests/master_tests.cpp, lines 2807-2809 https://reviews.apache.org/r/32463/diff/1/?file=904857#file904857line2807 Does it make sense to use `Clock::now().secs()` and compare doubles to avoid multiple conversions and mimic the way it is actually done in master code? Hm.. but you have to pause the clock, yes? I've gone with your suggestion since its cleaner, paused the Clock at the top of the test to ensure Clock::now matches the master's time. On March 25, 2015, 5:38 p.m., Alexander Rukletsov wrote: src/tests/slave_tests.cpp, line 937 https://reviews.apache.org/r/32463/diff/1/?file=904858#file904858line937 Kill `process::` prefix? Will leave the unrelated tests for now. :) - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32463/#review77738 --- On March 24, 2015, 11:42 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32463/ --- (Updated March 24, 2015, 11:42 p.m.) Review request for mesos, Alexander Rukletsov and Vinod Kone. Repository: mesos Description --- This improves the slave test, and adds a new test for the master, to help prevent API regressions. Diffs - src/tests/master_tests.cpp e69348be676a80017062e3abbd15b8008a6009d7 src/tests/slave_tests.cpp fd09d65bf34136c0959419b451e54105300740c4 Diff: https://reviews.apache.org/r/32463/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 32995: Split up documentation for reporting bugs and submitting patches.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32995/ --- Review request for mesos, Benjamin Hindman and Vinod Kone. Repository: mesos Description --- Developers guide was too generic, this splits up the documents to focus on what the reader is interesting in doing. Diffs - docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea docs/mesos-developers-guide.md 6023f2cc4d37ecabc24699bef6bda32068e5b43d docs/reporting-a-bug.md PRE-CREATION Diff: https://reviews.apache.org/r/32995/diff/ Testing --- N/A Thanks, Ben Mahler
Review Request 32997: Removed stale 'Code Internals' document.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32997/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- This document was fairly useless. In the future we should probably document our overall patterns (e.g. stout / libprocess). Diffs - docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea docs/mesos-code-internals.md 7e5b8972807408670f36c5664f5b95d52c1a9a51 Diff: https://reviews.apache.org/r/32997/diff/ Testing --- N/A Thanks, Ben Mahler
Review Request 32996: Updated roadmap document to link to 'Epic' tickets.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32996/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- Epic tickets provide a good view of upcoming and ongoing projects. Diffs - docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea docs/mesos-roadmap.md 150fad84d5ad264929775365fd1b941aefb89ec4 Diff: https://reviews.apache.org/r/32996/diff/ Testing --- N/A Thanks, Ben Mahler
Review Request 32999: Added a document for engineering principles and practices.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32999/ --- Review request for mesos, Adam B, Benjamin Hindman, Jie Yu, Niklas Nielsen, and Vinod Kone. Repository: mesos Description --- Added a document for engineering principles and practices. Diffs - docs/engineering-principles-and-practices.md PRE-CREATION docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea Diff: https://reviews.apache.org/r/32999/diff/ Testing --- N/A Thanks, Ben Mahler
Review Request 32998: Split committer's guide into code reviewing and committing documents.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32998/ --- Review request for mesos, Adam B, Benjamin Hindman, Jie Yu, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2581 https://issues.apache.org/jira/browse/MESOS-2581 Repository: mesos Description --- Committer's Guide was too generic. This names the documents after what the reader is looking for: doing effective reviews, and how to commit changes (for committers only). Diffs - docs/committers-guide.md c016ee9cb3290d7788ed258904547b59bbea4f11 docs/committing.md PRE-CREATION docs/effective-code-reviewing.md PRE-CREATION docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea Diff: https://reviews.apache.org/r/32998/diff/ Testing --- N/A Thanks, Ben Mahler
Re: Review Request 32939: Bumped the log level for dropped messages.
On April 8, 2015, 1:13 a.m., Ben Mahler wrote: 3rdparty/libprocess/src/process.cpp, line 2048 https://reviews.apache.org/r/32939/diff/1/?file=920067#file920067line2048 While you're here, how about: ``` VLOG(2) Dropping event for process: to; ``` If you want to get a bit fancier, we could pull up JSONVisitor and use it to print what we're dropping, but I doubt that be of much value right now. s/:// since ':' usually precedes a reason for the issue. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32939/#review79288 --- On April 7, 2015, 9:48 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32939/ --- (Updated April 7, 2015, 9:48 p.m.) Review request for mesos, Ben Mahler and Cody Maloney. Repository: mesos Description --- Currently these log messages are fired whenever we have delayed messages to a process that is exited (e.g., log appends). I've also seen this a bunch in our tests. Bumping this to VLOG(2) to keep fidelity of VLOG(1) high. Diffs - 3rdparty/libprocess/src/process.cpp cf4e36489be2c6aa01e838c1c71383f248deab5b Diff: https://reviews.apache.org/r/32939/diff/ Testing --- Thanks, Vinod Kone
Re: Review Request 32939: Bumped the log level for dropped messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32939/#review79288 --- Ship it! 3rdparty/libprocess/src/process.cpp https://reviews.apache.org/r/32939/#comment128538 While you're here, how about: ``` VLOG(2) Dropping event for process: to; ``` If you want to get a bit fancier, we could pull up JSONVisitor and use it to print what we're dropping, but I doubt that be of much value right now. - Ben Mahler On April 7, 2015, 9:48 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32939/ --- (Updated April 7, 2015, 9:48 p.m.) Review request for mesos, Ben Mahler and Cody Maloney. Repository: mesos Description --- Currently these log messages are fired whenever we have delayed messages to a process that is exited (e.g., log appends). I've also seen this a bunch in our tests. Bumping this to VLOG(2) to keep fidelity of VLOG(1) high. Diffs - 3rdparty/libprocess/src/process.cpp cf4e36489be2c6aa01e838c1c71383f248deab5b Diff: https://reviews.apache.org/r/32939/diff/ Testing --- Thanks, Vinod Kone
Review Request 32954: Added a 'slave_shutdowns_completed' metric.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32954/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- This allows us to determine the number of pending slave shutdowns, as the scheduled shutdowns must either be canceled or completed. Diffs - src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/master/metrics.hpp 52a83289cfe7e6b6fd8d5bff0774ebe5ce51d0ed src/master/metrics.cpp 14486bf7130250f561c9fb7a43c95f3fc1e76a4b Diff: https://reviews.apache.org/r/32954/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 32001: Required a period in trailing comments in the style guide.
On March 12, 2015, 9:20 p.m., Ben Mahler wrote: docs/mesos-c++-style-guide.md, line 30 https://reviews.apache.org/r/32001/diff/1/?file=892420#file892420line30 Hm, isn't this captured by ending each sentence with a period above? Alexander Rukletsov wrote: Yes and no. Trailing comments are not always sentences (at least in my non-native speaker's language model), it can be for example a JIRA ticket. As an alternative we can re-phrase the statement above to something like Each comment and sentences within it must end with a period. What do you think? Michael Park wrote: Haha I didn't even catch that: End each __sentence__ with a period. Do we just mean End each comment with a period.? Alexander Rukletsov wrote: Well, techically a comment may consist of multiple sentences, which means you can write something like this: ``` // To compensate for the case where a non-strict registrar is // being used, we explicitly deny removed slaves from // re-registering // This is because a non-strict registrar cannot // enforce this // We've already told frameworks the tasks were // lost so it's important to deny the slave from re-registering. ``` But maybe we lose too much time on this minor thing. Niklas Nielsen wrote: I would support mpark's idea here: How about having an extra bullet with End each sentence with a period.? Timothy Chen wrote: To be technically correct, we need a end each sentence with some puncatuation, as you'll also commonly see this in our code: master_tests.cpp // Step 7: Ensure the slave cannot re-register! Niklas Nielsen wrote: We got stuck here - BenM, would you prefer not to have the extra line? I think it is handy as we have seen the non-puncuation style bug quite a few times in comments. Well, I'm still a bit surprised that this level of detail is necessary. If you guys think it's helpful, the first iteration of this diff seems more succinct to me :) - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32001/#review76286 --- On March 26, 2015, 1:08 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32001/ --- (Updated March 26, 2015, 1:08 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary. Diffs - docs/mesos-c++-style-guide.md 439fe12ad137c1bed3a21d5a097c60a48f10a4f9 Diff: https://reviews.apache.org/r/32001/diff/ Testing --- None (not a functional change). Thanks, Alexander Rukletsov
Re: Review Request 32833: Added os::signals::install to install signal handlers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32833/#review79067 --- 3rdparty/libprocess/3rdparty/stout/include/stout/os/signals.hpp https://reviews.apache.org/r/32833/#comment128143 Why not use TryNothing? - Ben Mahler On April 3, 2015, 9:28 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32833/ --- (Updated April 3, 2015, 9:28 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- Added os::signals::install to install signal handlers. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os/signals.hpp 30232f50cc72a79acd21499fe7602c9bcd624ff6 Diff: https://reviews.apache.org/r/32833/diff/ Testing --- Tested in the later patch. Thanks, Jie Yu
Re: Review Request 32820: Fixed the non-POD global variable in perf sampler.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32820/#review78829 --- Ship it! Feel free to split out the proces namespace change. src/linux/perf.cpp https://reviews.apache.org/r/32820/#comment127885 char[] Also noticed many of our other constants are not static, we may want to do a sweep? - Ben Mahler On April 3, 2015, 5:01 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32820/ --- (Updated April 3, 2015, 5:01 p.m.) Review request for mesos, Ben Mahler and Ian Downes. Repository: mesos Description --- Fixed the non-POD global variable in perf sampler. Diffs - src/linux/perf.cpp cad6c80e2a9608ff02fc2b8976efba52713dd5a8 Diff: https://reviews.apache.org/r/32820/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32558: Improve compile time of mesos by splitting flags
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32558/#review78703 --- Looks good Cody, just curious about many of the includes in the .cpp flag files, had a hard time telling how these were relevant. src/master/flags.hpp https://reviews.apache.org/r/32558/#comment127653 Where is the mesos.hpp include for these? Looks like you removed it? src/master/flags.cpp https://reviews.apache.org/r/32558/#comment127655 I can't tell how these are relevant to this file, is there something I'm missing? src/master/flags.cpp https://reviews.apache.org/r/32558/#comment127656 How are these being used in this file? src/master/flags.cpp https://reviews.apache.org/r/32558/#comment127657 Ditto here, where is this relevant? src/slave/flags.cpp https://reviews.apache.org/r/32558/#comment127659 Why do you need this? src/slave/flags.cpp https://reviews.apache.org/r/32558/#comment127660 Why do you need this? src/slave/flags.cpp https://reviews.apache.org/r/32558/#comment127661 What's this one needed for? - Ben Mahler On March 31, 2015, 2:16 a.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32558/ --- (Updated March 31, 2015, 2:16 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Bugs: MESOS-292 https://issues.apache.org/jira/browse/MESOS-292 Repository: mesos Description --- Split the mesos master, slave flags into header + source file to improve compile time significantly. Should be no functional changes. Largely copy-paste, with a little reworking to remove unnecessary headers from the .hpp, and order headers a little more reliably (as well as remove duplicate includes) in the .cpp. # Impact of these changes `time make check -j8` Before: make check 2732.93s user 103.89s system 514% cpu 9:11.63 total After: make check 2421.18s user 96.60s system 506% cpu 8:16.67 total 4 core machine, 8 hypter threads enabled. SSD, 16 GB RAM, Intel i7-4790K. The numbers aren't incredibly stable (Other software running, overclocking enabled, etc). They are a good general measure though of speedup. I do a `make check` rather than just `make` because that is what devs do in a day-to-day flow, and if runtime is significantly impacted, it will show up. Test steps: ``` # Warm cache ../configure --disable-python --disable-java make check # Timed build rm -rf * ../configure --disable-python --disable-java time make check ``` Note: Similar, likely greater improvements in compile time would happen if stout were made to not be header only. Specifically stout/os.hpp. Diffs - src/Makefile.am 56ed9d9f047ebc3507c8ba235bba4e1ffcf76cd3 src/logging/flags.hpp 4facb33201cee5a82451a13ca05607c875574752 src/logging/flags.cpp PRE-CREATION src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/flags.hpp 85ce3285731c11b464aca6eaaa0a9165c73afebd src/master/flags.cpp PRE-CREATION src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef src/slave/flags.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32558/diff/ Testing --- make check on ArchLinux with GCC 4.9.2 make distcheck CentOS 7 Thanks, Cody Maloney
Re: Review Request 32694: Set death signal for forked du processes for posix/disk isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/#review78697 --- Ship it! src/slave/containerizer/isolators/posix/disk.cpp https://reviews.apache.org/r/32694/#comment127648 I'm still not sure why this is best-effort, since we never expect this to fail, can we just return its status code? It would be nice to find out if this isn't working as expected. This is even more important for 'perf' if we don't use the existing 'sleep' commend, since we don't want to run a long-running 'perf' if we can't set the death signal. - Ben Mahler On April 1, 2015, 7:40 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/ --- (Updated April 1, 2015, 7:40 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Set death signal for forked du processes for posix/disk isolator. Diffs - src/slave/containerizer/isolators/posix/disk.cpp 6e41e2a72cdcf914f2c922fdcb3d267b938e456e Diff: https://reviews.apache.org/r/32694/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32742: Added command logging for processes running in slave's cgroup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32742/#review78591 --- src/slave/slave.cpp https://reviews.apache.org/r/32742/#comment127484 ostringstream? Have you considered just building a setstring and using strings::join(\n, commands) below? Then you don't have to deal with the delimiters in the stringstream and the fact that we're left with an extra newline at the end as a result. - Ben Mahler On April 1, 2015, 8:05 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32742/ --- (Updated April 1, 2015, 8:05 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2461 https://issues.apache.org/jira/browse/MESOS-2461 Repository: mesos Description --- Added command logging for processes running in slave's cgroup. Diffs - src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09 Diff: https://reviews.apache.org/r/32742/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32698: Used the argv version of subprocess for linux perf utilities.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32698/#review78612 --- Ship it! src/linux/perf.cpp https://reviews.apache.org/r/32698/#comment127528 Do you need the named 'argv' or can you return the initializer list directly? - Ben Mahler On April 1, 2015, 7:40 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32698/ --- (Updated April 1, 2015, 7:40 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Used the argv version of subprocess for linux perf utilities. See discussion in MESOS-2462. This is to prepare for the death signal patch. Diffs - src/linux/perf.cpp 863aa4a972289a59f57e93cd06ba2bf9df949fe2 Diff: https://reviews.apache.org/r/32698/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 32742: Added command logging for processes running in slave's cgroup.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32742/#review78618
---
Ship it!
src/slave/slave.cpp
https://reviews.apache.org/r/32742/#comment127535
Could you just use \n since that's what we're joining with?
src/slave/slave.cpp
https://reviews.apache.org/r/32742/#comment127534
Is this a bit more succinct?
```
foreach (pid_t pid, processes.get()) {
Resultos::Process process = os::process(pid);
if (process.isSome()) {
infos.push_back(stringify(pid) + ' + process.get().command +
');
} else {
infos.push_back(stringify(pid));
}
}
```
- Ben Mahler
On April 1, 2015, 10:39 p.m., Jie Yu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32742/
---
(Updated April 1, 2015, 10:39 p.m.)
Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.
Bugs: MESOS-2461
https://issues.apache.org/jira/browse/MESOS-2461
Repository: mesos
Description
---
Added command logging for processes running in slave's cgroup.
Diffs
-
src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09
Diff: https://reviews.apache.org/r/32742/diff/
Testing
---
make check
Thanks,
Jie Yu
Re: Review Request 32694: Set death signal for forked du processes for posix/disk isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/#review78457 --- Seems like a decent approach for now, just a few minor notes that are applicable to the other reviews as well. src/slave/containerizer/isolators/posix/disk.cpp https://reviews.apache.org/r/32694/#comment127204 Can you just use an initializer list directly in the callsite here? src/slave/containerizer/isolators/posix/disk.cpp https://reviews.apache.org/r/32694/#comment127208 Can't you just bind directly into `prctl`? E.g. ``` Optionlambda::functionint() setup = None(); #ifdef __linux__ setup = lambda::bind(::prctl(PR_SET_PDEATHSIG, SIGKILL)); #endif TrySubprocess s = subprocess( du, argv, Subprocess::PATH(/dev/null), Subprocess::PIPE(), Subprocess::PIPE(), None(), None(), setup); ``` - Ben Mahler On March 31, 2015, 7:32 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32694/ --- (Updated March 31, 2015, 7:32 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Set death signal for forked du processes for posix/disk isolator. Diffs - src/slave/containerizer/isolators/posix/disk.cpp 6e41e2a72cdcf914f2c922fdcb3d267b938e456e Diff: https://reviews.apache.org/r/32694/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32502: Updated RECONCILE call to always specifiy slave id.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32502/#review78391 --- There is another nice aspect of requiring SlaveID in Reconcile and in Kill, we can make SlaveID in TaskStatus required. Are you planning to add a TODO for that? Will we be ever be able to make it required? include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32502/#comment127009 You can kill this now that we have a doc. include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32502/#comment127024 Should we make this a Reconcile::Task as opposed to a Reconcile::Status, now that it's no longer a TaskStatus? I'm also tempted to add a TODO for calling this 'Query' instead and allowing schedulers to query the master for additional things (other than tasks). src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32502/#comment127027 Shouldn't this come before you create the mesos object? src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32502/#comment127028 Shouldn't this come before you create the mesos object? Otherwise seems like you may receive the REGISTERED event before you set up this expectation? src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32502/#comment127031 newline here? src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32502/#comment127032 newline here? src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32502/#comment127030 newline here? src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32502/#comment127029 Do you want to check the reason? - Ben Mahler On March 31, 2015, 12:07 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32502/ --- (Updated March 31, 2015, 12:07 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Having a slave id will help us do better reconciliation. So added it to the 'Reconcile' call. Also updated the master to receive the Reconcile call directly. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32502/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32504: Removed MasterInfo from REGISTER and REREGISTER scheduler calls.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32504/#review78414 --- Ship it! Ship It! - Ben Mahler On March 31, 2015, 12:08 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32504/ --- (Updated March 31, 2015, 12:08 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Removed MasterInfo from REGISTER and REREGISTER scheduler calls because they don't provide much value in a HTTP API world because the scheduler knows precisely who it is connecting to. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a Diff: https://reviews.apache.org/r/32504/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32507: Moved REGISTER and REREGISTER out of scheduler Calls.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32507/#review78423 --- Instead of having a separate /scheduler/events endpoint with a top level Subscribe protobuf, could we just have one /scheduler/call endpoint and have a SUBSCRIBE Call? I prefer this because: (1) There is a single endpoint / Call interface for the scheduler. (2) We don't have to have the special Subscribe top level protobuf. (3) When we move to HTTP/2, we don't have to change anything, we can just tell people that they are now free to use a *single* connection for all calls. For HTTP/1.1, we tell people that they must make the streaming Subscribe call on a standalone connection. Was the motivation for separating the Subscribe call out to a separate endpoint to try to make the connection management more obvious? People already have to think about connection management regardless of having /scheduler/events as a different path. Are there any major reasons I'm missing for not consolidating on one endpoint? - Ben Mahler On March 31, 2015, 12:10 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32507/ --- (Updated March 31, 2015, 12:10 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOs-1127 https://issues.apache.org/jira/browse/MESOs-1127 Repository: mesos Description --- This is because subscription is done before schedulers can send any calls. Diffs - include/mesos/scheduler.hpp efee2cb5fc9f24e84294ed7e05a25cf8c81c2f1a include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/sched/sched.cpp 66fd2b3146568900356cc48e0f17c6720665ae80 src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32507/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32505: Added SHUTDOWN scheduler call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32505/#review78415 --- Ship it! Modulo comments. I noticed you added SlaveID on Shutdown, can you add it to Kill as well? include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32505/#comment127126 Do you want to put this below KILL? include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32505/#comment127142 Shuts down a custom executor When the executor include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32505/#comment127144 If the include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32505/#comment127146 transition include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32505/#comment127151 How about a newline for readability? ``` // transitions its active tasks to TASK_LOST. // // TODO(vinod): ... ``` include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32505/#comment127192 Should you add a required SlaveID to Kill as well since that's the same API as Shutdown? include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32505/#comment127161 Below Kill? src/master/master.hpp https://reviews.apache.org/r/32505/#comment127163 Above reconcile if you put it below Kill in the API? src/master/master.cpp https://reviews.apache.org/r/32505/#comment127164 Ditto here. src/master/master.cpp https://reviews.apache.org/r/32505/#comment127168 Whoops, this should already be saying either exited with status X or terminated with signal X. src/master/master.cpp https://reviews.apache.org/r/32505/#comment127170 Shouldn't this TODO be in the slave..? src/master/master.cpp https://reviews.apache.org/r/32505/#comment127171 Ditto about ordering w.r.t. other calls. src/master/master.cpp https://reviews.apache.org/r/32505/#comment127172 Did you forget the 'return' here? src/master/master.cpp https://reviews.apache.org/r/32505/#comment127174 Looks like you forgot to put the `return` statement here? :) It would be great if you could re-use `drop` here, either by doing the `Slave*` lookup in `receive`, much like we do the framework lookup and drop if not found, or by adding a `drop` overload. But the former seems cleaner. src/slave/slave.cpp https://reviews.apache.org/r/32505/#comment127181 shutdownFramework..? Do you need this comment now that you've split out a separate `_shutdownExecutor` function? src/slave/slave.cpp https://reviews.apache.org/r/32505/#comment127187 The top two LOG(WARNING)s are wrapped on the next line, but the next two are wrapped at the first line, mind just wrapping them all at the first line? src/slave/slave.cpp https://reviews.apache.org/r/32505/#comment127186 Mind putting the space in front of yet instead of at the end of not? Makes it a bit clearer that the spaces are correct IMHO because its quick to scan down a straight line with the eyes as opposed to the jagged end of the lines (and we do this above in this function :)). I remember I discussed wrapping || and conditionals at the front of the lines (for the same reason), but that ship has sailed.. src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32505/#comment127188 Ditto from my other review, don't you need this above the Mesos construction? src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32505/#comment127189 newline here? src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32505/#comment127190 newline here? src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32505/#comment127191 newline here? - Ben Mahler On March 31, 2015, 12:09 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32505/ --- (Updated March 31, 2015, 12:09 a.m.) Review request for mesos, Benjamin Hindman and Ben Mahler. Bugs: MESOS-1127 and MESOS-330 https://issues.apache.org/jira/browse/MESOS-1127 https://issues.apache.org/jira/browse/MESOS-330 Repository: mesos Description --- This is a new call added to explicitly shutdown an executor. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae src
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
On Feb. 18, 2015, 7:38 p.m., Niklas Nielsen wrote: Let's get tests wired up before committing this :) Adam B wrote: Sure thing. Adding tests in my subsequent patch where we will pass the master's timeout values on to the slave. Will post that very soon. Ben Mahler wrote: Can you do it in one patch? This patch in isolation looks a bit dangerous per our conversation above. Also, please carefully consider whether your approach will be safe to do in a single version. i.e. What happens when there are old slaves running against a new master? And vice versa. Adam B wrote: Easy enough. Added the second patch to this one. Most of the new changes are in messages.proto, slave.hpp/cpp, and partition_tests.cpp, with a few lines in master.cpp to set the new message fields. Certainly safe to upgrade if the new flags stay at their default value. Also, with new slaves talking to an old master, the master will still use the defaults, hence so will the slaves. But old slaves running against a new master with a longer timeout will try to reregister unnecessarily early, so you may want to guarantee that all/most of the slaves have been upgraded before setting these flags, otherwise a large cluster suddenly waking up from a network split would see a lot of unnecessary reregistration attempts. The old behavior in this scenario was that the slaves would reregister after the same default timeout after the network split, and the master would consider them shutdown after the same timeout. If that last scenario is a problem, maybe the better approach is for each slave to specify its own ping timeout, and then the master can use these values with each slave's SlaveObserver. Then we can just require the master(s) to be upgraded first, as is typical. Adam B wrote: I'm inclined to think that we shouldn't worry too much about the unlikely scenario of a network split in the middle of a rolling upgrade to 0.23 where longer ping timeouts are simultaneously being applied. Procedure should be: upgrade (most of) the cluster, then restart the master(s) with the new longer ping timeout value. I can add a Note to upgrades.md if you like. How does that sound to you @bmahler ? If there are no objections, I'll rebase the patch and update the configuration and upgrades docs. Yeah that sounds fine to me (might be prudent to call it out in the upgrade doc or the changelog). Mind reaching out to me directly when this is ready for more reviewing? :) - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29507/#review72992 --- On Feb. 19, 2015, 8:10 a.m., Adam B wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29507/ --- (Updated Feb. 19, 2015, 8:10 a.m.) Review request for mesos, Ben Mahler and Niklas Nielsen. Bugs: MESOS-2110 https://issues.apache.org/jira/browse/MESOS-2110 Repository: mesos Description --- Added new --slave_ping_timeout and --max_slave_ping_timeouts flags to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs) and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5). These can be extended if slaves are expected/allowed to be down for longer than a minute or two. Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage. Beware that this affects recovery from network timeouts as well as actual slave node/process failover. Diffs - src/master/constants.hpp ad3fe81 src/master/constants.cpp d3d0f71 src/master/flags.hpp 51a6059 src/master/master.cpp f10a3cf src/messages/messages.proto 58484ae src/slave/constants.hpp 12d6e92 src/slave/constants.cpp 7868bef src/slave/slave.hpp 91dae10 src/slave/slave.cpp aec9525 src/tests/fault_tolerance_tests.cpp efa5c57 src/tests/partition_tests.cpp eb16a58 src/tests/slave_recovery_tests.cpp 8210c52 src/tests/slave_tests.cpp 153d9d6 Diff: https://reviews.apache.org/r/29507/diff/ Testing --- Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts. Ran unit tests with shorter non-default values for ping timeouts. `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave Thanks, Adam B
Re: Review Request 32346: Added failure semantics for http::Pipe::Writer.
On March 25, 2015, 10:12 p.m., Jie Yu wrote: 3rdparty/libprocess/include/process/http.hpp, line 173 https://reviews.apache.org/r/32346/diff/1/?file=901898#file901898line173 Indent. Weird, the indent is actually ok but in reviewboard it looks offset due to the moved from ui component. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32346/#review77802 --- On March 20, 2015, 11:51 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32346/ --- (Updated March 20, 2015, 11:51 p.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- The Pipe writer needs to be able to fail the reader when it cannot complete the pipe. For example, if we are decoding a response and we cannot parse the body, we must ensure the reader of the body sees a failure isntead of EOF. This was used in the subsequent patches in this chain. Diffs - 3rdparty/libprocess/include/process/http.hpp 2b366986b1f617e95bda94e07f2a0e532f5626f6 3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef 3rdparty/libprocess/src/tests/http_tests.cpp 17fb092a851e128c137152fdce57e9fb10a08bf7 Diff: https://reviews.apache.org/r/32346/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 32347: Added a StreamingResponseDecoder.
On March 27, 2015, 12:20 a.m., Jie Yu wrote: 3rdparty/libprocess/src/tests/decoder_tests.cpp, lines 177-178 https://reviews.apache.org/r/32347/diff/1/?file=901908#file901908line177 Do you want to move this up right after ``` decoder.decode(body.data(), body.length()); ``` ? Sounds better, thanks! On March 27, 2015, 12:20 a.m., Jie Yu wrote: 3rdparty/libprocess/src/decoder.hpp, line 480 https://reviews.apache.org/r/32347/diff/1/?file=901907#file901907line480 Looks like we won't be using the old ResponseDecoder anymore after https://reviews.apache.org/r/32351. Are you planing to kill the old response decoder? If yes, the name Streaming here seems to be uncessary once the old response decoder is killed. Are you going to s/StreamingResponseDecoder/ResponseDecoder/ after r32351 is committed? Yeah, I'll remove the old decoder in a follow up change, thanks! - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32347/#review77967 --- On March 20, 2015, 11:51 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32347/ --- (Updated March 20, 2015, 11:51 p.m.) Review request for mesos, Benjamin Hindman and Jie Yu. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- Provides a response decoder that returns 'PIPE' responses once the response headers are received, but before the body data is received. Callers are expected to read the body from the Pipe::Reader in the response. This is needed to receive streaming responses on the client-side. Diffs - 3rdparty/libprocess/src/decoder.hpp 45f3d7fff2c33ce9c37a8ecbdcbf2c0f02ee76e8 3rdparty/libprocess/src/tests/decoder_tests.cpp d65f5cf25eaecd5994404ccdf07f32d703a5955b Diff: https://reviews.apache.org/r/32347/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 32558: Improve compile time of mesos by splitting flags
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32558/#review78303 --- Great work Cody! For posterity, how did you pinpoint flags.hpp? - Ben Mahler On March 27, 2015, 1:21 a.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32558/ --- (Updated March 27, 2015, 1:21 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Bugs: MESOS-292 https://issues.apache.org/jira/browse/MESOS-292 Repository: mesos Description --- Split the mesos master, slave flags into header + source file to improve compile time significantly. Should be no functional changes. Largely copy-paste, with a little reworking to remove unnecessary headers from the .hpp, and order headers a little more reliably (as well as remove duplicate includes) in the .cpp. # Impact of these changes `time make check -j8` Before: make check 2732.93s user 103.89s system 514% cpu 9:11.63 total After: make check 2421.18s user 96.60s system 506% cpu 8:16.67 total 4 core machine, 8 hypter threads enabled. SSD, 16 GB RAM, Intel i7-4790K. The numbers aren't incredibly stable (Other software running, overclocking enabled, etc). They are a good general measure though of speedup. I do a `make check` rather than just `make` because that is what devs do in a day-to-day flow, and if runtime is significantly impacted, it will show up. Test steps: ``` # Warm cache ../configure --disable-python --disable-java make check # Timed build rm -rf * ../configure --disable-python --disable-java time make check ``` Note: Similar, likely greater improvements in compile time would happen if stout were made to not be header only. Specifically stout/os.hpp. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/logging/flags.hpp 4facb33201cee5a82451a13ca05607c875574752 src/logging/flags.cpp PRE-CREATION src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/flags.hpp 85ce3285731c11b464aca6eaaa0a9165c73afebd src/master/flags.cpp PRE-CREATION src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef src/slave/flags.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32558/diff/ Testing --- make check on ArchLinux with GCC 4.9.2 make distcheck CentOS 7 Thanks, Cody Maloney
Re: Review Request 32351: Added http::streaming::get/post for client-side streaming responses.
On March 30, 2015, 6:40 p.m., Jie Yu wrote:
3rdparty/libprocess/include/process/http.hpp, line 587
https://reviews.apache.org/r/32351/diff/2/?file=903535#file903535line587
Can you put a TODO here you wrote in the description of this review:
```
Long term I'd like to just have a http::request(..., Request) function
rather than the overloads.
```
There is an existing TODO above all of these:
```
// TODO(bmahler): Consolidate these functions into a single
// http::request function that takes a 'Request' object.
```
On March 30, 2015, 6:40 p.m., Jie Yu wrote:
3rdparty/libprocess/src/http.cpp, line 659
https://reviews.apache.org/r/32351/diff/2/?file=903536#file903536line659
Can you handle the DISCARDED case as well (instead of a CHECK). We
never know if the impl of the socket recv will change or not.
```
if (!data.isReady()) {
// XXX
responses = decoder-decode(, 0);
} else {
...
}
```
Sure, although I thought we wanted DISCARDED to only occur when a caller
requests a discard..?
On March 30, 2015, 6:40 p.m., Jie Yu wrote:
3rdparty/libprocess/src/http.cpp, line 828
https://reviews.apache.org/r/32351/diff/2/?file=903536#file903536line828
Why change the name to `internal::_recv`?
This function tries to decode the content received from the socket.
Using the name `internal::decode` seems to be more appropriate.
This was because previously `decode` only performed decoding. Now we need to
continue receiving from the socket inside both `_recv` and `__recv`.
I agree it is less intuitive though, so I've renamed them to `decode` and
`_decode`. Thanks!
On March 30, 2015, 6:40 p.m., Jie Yu wrote:
3rdparty/libprocess/src/http.cpp, lines 551-558
https://reviews.apache.org/r/32351/diff/2/?file=903536#file903536line551
I would rather reorder the helpers so that I don't need to do forward
declarations.
Hm.. looking around the code, it seems like we maintain the continuation order
even if it means forward declaring.
- Ben
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32351/#review78111
---
On March 23, 2015, 11 p.m., Ben Mahler wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32351/
---
(Updated March 23, 2015, 11 p.m.)
Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere.
Bugs: MESOS-2438
https://issues.apache.org/jira/browse/MESOS-2438
Repository: mesos
Description
---
The caller must make the decision as to whether it expects to stream the
response via PIPE or get the full BODY response, otherwise they must
implement both cases, which is pretty hacky.
So the approach here is to introduce a streaming:: namespace for this. Long
term I'd like to just have a http::request(..., Request) function rather than
the overloads.
Diffs
-
3rdparty/libprocess/include/process/http.hpp
2b366986b1f617e95bda94e07f2a0e532f5626f6
3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef
3rdparty/libprocess/src/tests/http_tests.cpp
17fb092a851e128c137152fdce57e9fb10a08bf7
Diff: https://reviews.apache.org/r/32351/diff/
Testing
---
Added a test.
Thanks,
Ben Mahler
Re: Review Request 32550: Add major, minor, and patch accessors to stout's Version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32550/#review77992 --- I'm curious, what do you need these for? We intentionally left them out, because we figured comparisons would be used rather than explicit checks against numbers. - Ben Mahler On March 27, 2015, 12:01 a.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32550/ --- (Updated March 27, 2015, 12:01 a.m.) Review request for mesos, Benjamin Hindman and Niklas Nielsen. Bugs: MESOS-1795 and MESOS-2161 https://issues.apache.org/jira/browse/MESOS-1795 https://issues.apache.org/jira/browse/MESOS-2161 Repository: mesos Description --- We called them majorVersion, minorVersion, and patchVersion to avoid conflicting with major() and minor() (http://man.he.net/man3/gnu_dev_major) Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 090fcf09dd96538a8748cf4443d150911e2c0d27 Diff: https://reviews.apache.org/r/32550/diff/ Testing --- Thanks, Joris Van Remoortere
Review Request 32555: Fixed the rate limiting of slave removals during master recovery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32555/ --- Review request for mesos, Jie Yu and Vinod Kone. Bugs: MESOS-2392 https://issues.apache.org/jira/browse/MESOS-2392 Repository: mesos Description --- When we adding rate limiting of recovered slave removals, the metric logic was buggy in that it reports all of the recovered slaves as having been scheduled for removal. It also unnecessarily acquires rate limiting tokens. Diffs - src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 Diff: https://reviews.apache.org/r/32555/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 32495: Fixed a regression caused by multiple cgroups base hierarchy detection.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32495/#review77795 --- Mind adding how this fixes the issue to the description? - Ben Mahler On March 25, 2015, 9:02 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32495/ --- (Updated March 25, 2015, 9:02 p.m.) Review request for mesos, Ben Mahler and Ian Downes. Bugs: MESOS-2548 https://issues.apache.org/jira/browse/MESOS-2548 Repository: mesos Description --- Fixed a regression caused by multiple cgroups base hierarchy detection. Diffs - src/tests/mesos.cpp dc2a906a57b9abace4713aea8b0a47fec1c1a8a4 Diff: https://reviews.apache.org/r/32495/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 32495: Fixed a regression caused by multiple cgroups base hierarchy detection.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32495/#review77797 --- Ship it! Thanks! - Ben Mahler On March 25, 2015, 9:12 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32495/ --- (Updated March 25, 2015, 9:12 p.m.) Review request for mesos, Ben Mahler and Ian Downes. Bugs: MESOS-2548 https://issues.apache.org/jira/browse/MESOS-2548 Repository: mesos Description --- Fixed a regression caused by multiple cgroups base hierarchy detection. The previous way of getting base hierarchy is not reliable in the case of cgroup co-mounts and symlinks (e.g. systemd). For example, you may see the following layout: ``` /sys/fs/cgroup/cpu,cpuacct /sys/fs/cgroup/cpu - /sys/fs/cgroup/cpu,cpuacct /sys/fs/cgroup/cpuacct - /sys/fs/cgroup/cpu,cpuacct ``` For subsystem 'cpuacct', the current code will report the base hierarchy to be '/sys/fs/cgroup/cpu,'. Diffs - src/tests/mesos.cpp dc2a906a57b9abace4713aea8b0a47fec1c1a8a4 Diff: https://reviews.apache.org/r/32495/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 30546: MemIsolator: expose memory pressures for containers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30546/#review77820 --- Looks great Chi, can you adjust this per my comments on the depedent review about renaming the Listener to Counter? include/mesos/mesos.proto https://reviews.apache.org/r/30546/#comment126116 It wasn't obvious to me initially that these reset to 0 when the slave restarts, could we add a brief note about that? src/slave/containerizer/isolators/cgroups/mem.cpp https://reviews.apache.org/r/30546/#comment126128 Brace on the next line, also, why the trailing semi-colon? src/slave/containerizer/isolators/cgroups/mem.cpp https://reviews.apache.org/r/30546/#comment126117 Could you remove the trailing period here? Also, any reason you're not including the listener.error() in the message? Seems useful. src/slave/containerizer/isolators/cgroups/mem.cpp https://reviews.apache.org/r/30546/#comment126125 It looks like the isolators have become inconsistent about what they return for an unknown container (e.g. network isolator is not returning a failure for unknown container, but the memory/cpu isolators are). Much like in `usage`, let's just return a Failure here rather than an empty result. The containerizer should understand that it destroyed this container and doesn't need the usage() result anymore, right? src/slave/containerizer/isolators/cgroups/mem.cpp https://reviews.apache.org/r/30546/#comment126123 Can you indent the cases? Also, there is a bug here, I'll let you find it ;) src/slave/containerizer/isolators/cgroups/mem.cpp https://reviews.apache.org/r/30546/#comment126122 Not returning a Failure in this case (like the file parsing cases in `usage()` above) seems to suggest that we're not sure if the pressure counters can fail and so we want to proceed with the partial result. That's good, but why not keep logging the error? (rather than erasing the counter upon the first issue we see). Since we're not expecting this to be possible (but we're not confident enough about eventfds to make this a Failure()) let's just print an ERROR each time we are returning a partial result, to make this more obvious in the logs. This also ends up cleaning up the code, as you no longer need to keep an iterator around for erasing. src/slave/containerizer/isolators/cgroups/mem.cpp https://reviews.apache.org/r/30546/#comment126127 Do you need to print this? Seems unnecessary? - Ben Mahler On March 25, 2015, 12:03 a.m., Chi Zhang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30546/ --- (Updated March 25, 2015, 12:03 a.m.) Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu. Bugs: MESOS-2136 https://issues.apache.org/jira/browse/MESOS-2136 Repository: mesos Description --- MemIsolator: expose memory pressures for containers. Diffs - include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 src/slave/containerizer/isolators/cgroups/mem.hpp a00f723de61b9bccd76a2948b6d14fde7a993a8d src/slave/containerizer/isolators/cgroups/mem.cpp eaeb30164e8ae8c0a703683b7cc0bf1851760c95 src/tests/slave_recovery_tests.cpp 87f4a6aab27d142fa8eb7a6571f684a6ce59687e Diff: https://reviews.apache.org/r/30546/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 30545: cgroups: added support to listen on memory pressures.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30545/#review77819 --- Looks good, just a minor API suggestion below. src/linux/cgroups.hpp https://reviews.apache.org/r/30545/#comment126114 One initial confusion for me was that Listener seemed to suggest that it listens and tells us when an event occurs. That's generally how we've used the word listener. However, this is merely keeping a count on behalf of the caller. The caller cannot receive events from this. So, could we just call this a `memory::pressure::Counter`, where you can get the value of the counter as follows: `Futureuint64_t value = counter.value()`? So rather than having a Listener that provides its counter, we just have a Counter that provides its value. I think that will make this a little more intuitive, thoughts? - Ben Mahler On March 11, 2015, 10:30 p.m., Chi Zhang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30545/ --- (Updated March 11, 2015, 10:30 p.m.) Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu. Bugs: MESOS-2136 https://issues.apache.org/jira/browse/MESOS-2136 Repository: mesos Description --- cgroups: added support to listen on memory pressures. Diffs - src/linux/cgroups.hpp e07772fec11b9bb73a44c54326f24d7ee1e8a64b src/linux/cgroups.cpp a533b319fc75abb0fc45b8f5f473f257912d21ac Diff: https://reviews.apache.org/r/30545/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 32501: Removed REQUEST call from scheduler.proto.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32501/#review77842 --- Ship it! Should you be adding a TODO on 'Request' to remove it when the old driver goes away? Or is there a reason to leave it indefinitely? - Ben Mahler On March 25, 2015, 11:07 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32501/ --- (Updated March 25, 2015, 11:07 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Removed REQUEST call because it is unused. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a Diff: https://reviews.apache.org/r/32501/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32506: Added output stream operators for scheduler Calls and Events.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32506/#review77847 --- Ship it! Ship It! - Ben Mahler On March 25, 2015, 11:13 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32506/ --- (Updated March 25, 2015, 11:13 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- For readable output in logs. Diffs - include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 Diff: https://reviews.apache.org/r/32506/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32500: Removed LAUNCH call from scheduler.proto.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32500/#review77836 --- Ship it! include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32500/#comment126165 Ditto here, the tag numbers don't matter much, but are you planning to fill in the holes since we're breaking this? include/mesos/scheduler/scheduler.proto https://reviews.apache.org/r/32500/#comment126164 Just curious, are you planning on updating the tag numbers at some point or just going to leave the holes? src/examples/low_level_scheduler_libprocess.cpp https://reviews.apache.org/r/32500/#comment126163 Ditto here, maybe some newlines to make this a bit more readable? src/examples/low_level_scheduler_pthread.cpp https://reviews.apache.org/r/32500/#comment126160 newline here? src/examples/low_level_scheduler_pthread.cpp https://reviews.apache.org/r/32500/#comment126161 newline here? src/examples/low_level_scheduler_pthread.cpp https://reviews.apache.org/r/32500/#comment126162 newline here? src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32500/#comment126158 newline here? src/tests/scheduler_tests.cpp https://reviews.apache.org/r/32500/#comment126159 newline here? - Ben Mahler On March 25, 2015, 11:06 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32500/ --- (Updated March 25, 2015, 11:06 p.m.) Review request for mesos, Ben Mahler and Jie Yu. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Removed LAUNCH call in favor of ACCEPT. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32500/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32420: Fix PerfTest.ROOT_SampleInit to sample a process known to consume cpu.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32420/#review77664 --- Ship it! src/tests/perf_tests.cpp https://reviews.apache.org/r/32420/#comment125832 If `prctl` fails, can you exit? - Ben Mahler On March 24, 2015, 7:06 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32420/ --- (Updated March 24, 2015, 7:06 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2534 https://issues.apache.org/jira/browse/MESOS-2534 Repository: mesos Description --- init/systemd/launchd doesn't always run within a reasonable length interval. Changed test to fork and sample a child that spins. Reduced the test time too. Diffs - src/tests/perf_tests.cpp 16f32267f694ccaae9d10c2629831e2cca23e0c8 Diff: https://reviews.apache.org/r/32420/diff/ Testing --- ./bin/mesos-tests.sh --gtest_filter=PerfTest.ROOT_SamplePid --gtest_repeat=100 Thanks, Ian Downes
Re: Review Request 32419: Used move assignment in the master's json endpoints.
On March 24, 2015, 9:32 p.m., Michael Park wrote:
src/common/http.cpp, lines 179-187
https://reviews.apache.org/r/32419/diff/1/?file=903656#file903656line179
Just a suggestion for an alternative pattern here.
Rather than
```
{
JSON::Array array;
array.values.reserve(size);
/* Fill in array... */
object.values[statuses] = std::move(array);
}
```
How about:
```
{
JSON::Array array = object.values[statuses];
array.values.reserve(size);
/* Fill in array... */
}
```
Seems to me like this code is just as readable and doing no work is
better than doing some work.
`object.values[statuses]` is a variant JSON::Value (defaults to JSON::Null
IIUC), not a JSON::Array, does that compile..?
- Ben
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32419/#review77657
---
On March 24, 2015, 12:27 a.m., Ben Mahler wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32419/
---
(Updated March 24, 2015, 12:27 a.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-2353
https://issues.apache.org/jira/browse/MESOS-2353
Repository: mesos
Description
---
This is a simple attempt at improvement for MESOS-2353 in the interim of
writing a more comprehensive JSON benchmark.
Per the discussion on [r/31700](https://reviews.apache.org/r/31700/), moves
here avoid the very expensive copies of the large JSON objects so long as
variant has move assignment defined (or generated) and our objects have move
generated.
It appears that variant has move assignment and our json classes can have
move assignment generated, so this should help.
Diffs
-
src/common/http.cpp d3b7ca3727567ff252f978720f518ad0912e533b
src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401
Diff: https://reviews.apache.org/r/32419/diff/
Testing
---
make check
No benchmarking performed given the analysis from Michael in
[r/31700](https://reviews.apache.org/r/31700/). I will also be trying this
out on our large production clusters.
Thanks,
Ben Mahler
Re: Review Request 32419: Used move assignment in the master's state.json endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32419/ --- (Updated March 24, 2015, 11:42 p.m.) Review request for mesos and Jie Yu. Changes --- * Fixed an accidental shadowing issue. Summary (updated) - Used move assignment in the master's state.json endpoint. Bugs: MESOS-2353 https://issues.apache.org/jira/browse/MESOS-2353 Repository: mesos Description --- This is a simple attempt at improvement for MESOS-2353 in the interim of writing a more comprehensive JSON benchmark. Per the discussion on [r/31700](https://reviews.apache.org/r/31700/), moves here avoid the very expensive copies of the large JSON objects so long as variant has move assignment defined (or generated) and our objects have move generated. It appears that variant has move assignment and our json classes can have move assignment generated, so this should help. Diffs (updated) - src/common/http.cpp d3b7ca3727567ff252f978720f518ad0912e533b src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 Diff: https://reviews.apache.org/r/32419/diff/ Testing --- make check No benchmarking performed given the analysis from Michael in [r/31700](https://reviews.apache.org/r/31700/). I will also be trying this out on our large production clusters. Thanks, Ben Mahler
Re: Review Request 32466: Cleaned up and simplified PortMappingMesosTest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32466/#review77692 --- Ship it! Modulo the questions below, ROOT_CleanUpOrphanTest looks good though. src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32466/#comment125874 Why wasn't this needed before? src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32466/#comment125878 Why do you need this if you're using WillRepeatedly? src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32466/#comment125879 Might want to confirm this compiles on 4.4 if you can. src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32466/#comment125880 Why do you need the filters here? src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32466/#comment125881 What's this for? Can you add a comment for posterity? (i.e. why is the settle needed before the advance here). src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32466/#comment125885 Can we keep the comment you removed above but have it down here? e.g. ``` // Ensure that both containers (with and without network isolation) were recovered. ``` src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32466/#comment125882 Not yours, but const ? src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32466/#comment125886 What was this trying to test? src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32466/#comment125875 Why wasn't this needed before? src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32466/#comment125876 Might want to check that this compiles on 4.4. src/tests/port_mapping_tests.cpp https://reviews.apache.org/r/32466/#comment125877 You don't need the 'true' here. - Ben Mahler On March 25, 2015, 12:04 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32466/ --- (Updated March 25, 2015, 12:04 a.m.) Review request for mesos, Ben Mahler, Chi Zhang, and Vinod Kone. Repository: mesos Description --- Cleaned up and simplified PortMappingMesosTest. Diffs - src/tests/port_mapping_tests.cpp 623840e71938791a562a32989775818275e6d37e Diff: https://reviews.apache.org/r/32466/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 32467: Allowed MesosContainerizer to take empty isolation flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32467/#review77685 --- Ship it! Ship It! - Ben Mahler On March 25, 2015, 12:04 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32467/ --- (Updated March 25, 2015, 12:04 a.m.) Review request for mesos, Ben Mahler and Ian Downes. Repository: mesos Description --- Allowed MesosContainerizer to take empty isolation flag. Diffs - src/slave/containerizer/mesos/containerizer.cpp fbd1c0a0e5f4f227adb022f0baaa6d2c7e3ad748 Diff: https://reviews.apache.org/r/32467/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32419: Used move assignment in the master's json endpoints.
On March 24, 2015, 4:10 p.m., Alexander Rukletsov wrote: src/master/http.cpp, line 470 https://reviews.apache.org/r/32419/diff/1/?file=903657#file903657line470 Are you sure the outer `object` is not shadowed? Even if it's not, let us maybe leave the original naming to avoid name collision and therefore confusion. Yikes, good eye alex, this looks like a bug. I'll leave it as it was, or I'll update the names to be flags, slaves, etc instead of object, array, etc. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32419/#review77585 --- On March 24, 2015, 12:27 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32419/ --- (Updated March 24, 2015, 12:27 a.m.) Review request for mesos and Jie Yu. Bugs: MESOS-2353 https://issues.apache.org/jira/browse/MESOS-2353 Repository: mesos Description --- This is a simple attempt at improvement for MESOS-2353 in the interim of writing a more comprehensive JSON benchmark. Per the discussion on [r/31700](https://reviews.apache.org/r/31700/), moves here avoid the very expensive copies of the large JSON objects so long as variant has move assignment defined (or generated) and our objects have move generated. It appears that variant has move assignment and our json classes can have move assignment generated, so this should help. Diffs - src/common/http.cpp d3b7ca3727567ff252f978720f518ad0912e533b src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 Diff: https://reviews.apache.org/r/32419/diff/ Testing --- make check No benchmarking performed given the analysis from Michael in [r/31700](https://reviews.apache.org/r/31700/). I will also be trying this out on our large production clusters. Thanks, Ben Mahler
Re: Review Request 32419: Used move assignment in the master's json endpoints.
On March 24, 2015, 4:10 p.m., Alexander Rukletsov wrote: src/master/http.cpp, line 470 https://reviews.apache.org/r/32419/diff/1/?file=903657#file903657line470 Are you sure the outer `object` is not shadowed? Even if it's not, let us maybe leave the original naming to avoid name collision and therefore confusion. Ben Mahler wrote: Yikes, good eye alex, this looks like a bug. I'll leave it as it was, or I'll update the names to be flags, slaves, etc instead of object, array, etc. I'll also add a test for the miscellaneous items always present in state.json, if one doesn't exist already. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32419/#review77585 --- On March 24, 2015, 12:27 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32419/ --- (Updated March 24, 2015, 12:27 a.m.) Review request for mesos and Jie Yu. Bugs: MESOS-2353 https://issues.apache.org/jira/browse/MESOS-2353 Repository: mesos Description --- This is a simple attempt at improvement for MESOS-2353 in the interim of writing a more comprehensive JSON benchmark. Per the discussion on [r/31700](https://reviews.apache.org/r/31700/), moves here avoid the very expensive copies of the large JSON objects so long as variant has move assignment defined (or generated) and our objects have move generated. It appears that variant has move assignment and our json classes can have move assignment generated, so this should help. Diffs - src/common/http.cpp d3b7ca3727567ff252f978720f518ad0912e533b src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 Diff: https://reviews.apache.org/r/32419/diff/ Testing --- make check No benchmarking performed given the analysis from Michael in [r/31700](https://reviews.apache.org/r/31700/). I will also be trying this out on our large production clusters. Thanks, Ben Mahler
Re: Review Request 32452: Disallowed multiple cgroups base hierarchies in tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32452/#review77627 --- Ship it! Ship It! - Ben Mahler On March 24, 2015, 7:47 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32452/ --- (Updated March 24, 2015, 7:47 p.m.) Review request for mesos, Ben Mahler and Ian Downes. Repository: mesos Description --- Disallowed multiple cgroups base hierarchies in tests. Right now, if some subsystems are pre-mounted at /sys/fs/cgroup and some of them are not pre-mounted, the cgroups test will fail depending on the order in which the susbsystems we checked. For instance, if the first subsystem we check is not pre-mounted, we will set the base hierarchy to TEST_CGROUPS_HIERARCHY. ANd we'll assume all subsystems are under TEST_CGROUPS_HIERARCHY, which might not be true. This patch disallows the case where multiple base hierarchies exist. Diffs - src/tests/mesos.cpp 11e88330dd50913ab00da79054225d113541e83e Diff: https://reviews.apache.org/r/32452/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 32384: Adding perf check to configure
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32384/#review77469 --- Perf tests should just be disabled if it's not present, but I don't see a filter for this currently: https://github.com/apache/mesos/blob/master/src/tests/environment.cpp Also keep in mind perf is only relevant for Linux. - Ben Mahler On March 23, 2015, 3:42 a.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32384/ --- (Updated March 23, 2015, 3:42 a.m.) Review request for mesos and Cody Maloney. Bugs: MESOS-2166 https://issues.apache.org/jira/browse/MESOS-2166 Repository: mesos-incubating Description --- PerfEventIsolatorTest.ROOT_CGROUPS_Sample requires 'perf' to be installed Diffs - configure.ac 9b2d7f1 Diff: https://reviews.apache.org/r/32384/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 32351: Added http::streaming::get/post for client-side streaming responses.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32351/ --- (Updated March 23, 2015, 11 p.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere. Changes --- * Fixed a compilation issue. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- The caller must make the decision as to whether it expects to stream the response via PIPE or get the full BODY response, otherwise they must implement both cases, which is pretty hacky. So the approach here is to introduce a streaming:: namespace for this. Long term I'd like to just have a http::request(..., Request) function rather than the overloads. Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp 2b366986b1f617e95bda94e07f2a0e532f5626f6 3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef 3rdparty/libprocess/src/tests/http_tests.cpp 17fb092a851e128c137152fdce57e9fb10a08bf7 Diff: https://reviews.apache.org/r/32351/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 32420: Fix PerfTest.ROOT_SampleInit to sample a process known to consume cpu.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32420/#review77507 --- Looks good! Just hoping we can avoid the process leak. src/tests/perf_tests.cpp https://reviews.apache.org/r/32420/#comment125603 Can we guarantee that we don't leak a long-lived process? Since you're using linux, we could manually use PR_SET_PDEATHSIG here (I assume this works when the parent is already dead..?). Otherwsie, we can make this loop for a limited duration of time. Just something to prevent this from leaking if the test fails, crashes, or is killed before we get a change to SIGKILL the child. - Ben Mahler On March 24, 2015, 12:17 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32420/ --- (Updated March 24, 2015, 12:17 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2534 https://issues.apache.org/jira/browse/MESOS-2534 Repository: mesos Description --- init/systemd/launchd doesn't always run within a reasonable length interval. Changed test to fork and sample a child that spins. Reduced the test time too. Diffs - src/tests/perf_tests.cpp 16f32267f694ccaae9d10c2629831e2cca23e0c8 Diff: https://reviews.apache.org/r/32420/diff/ Testing --- ./bin/mesos-tests.sh --gtest_filter=PerfTest.ROOT_SamplePid --gtest_repeat=100 Thanks, Ian Downes
Review Request 32419: Used move assignment in the master's json endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32419/ --- Review request for mesos and Jie Yu. Bugs: MESOS-2353 https://issues.apache.org/jira/browse/MESOS-2353 Repository: mesos Description --- This is a simple attempt at improvement for MESOS-2353 in the interim of writing a more comprehensive JSON benchmark. Per the discussion on [r/31700](https://reviews.apache.org/r/31700/), moves here avoid the very expensive copies of the large JSON objects so long as variant has move assignment defined (or generated) and our objects have move generated. It appears that variant has move assignment and our json classes can have move assignment generated, so this should help. Diffs - src/common/http.cpp d3b7ca3727567ff252f978720f518ad0912e533b src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 Diff: https://reviews.apache.org/r/32419/diff/ Testing --- make check No benchmarking performed given the analysis from Michael in [r/31700](https://reviews.apache.org/r/31700/). I will also be trying this out on our large production clusters. Thanks, Ben Mahler
Re: Review Request 31930: Introduced an http::Pipe abstraction to simplify streaming HTTP Responses.
On March 18, 2015, 11:49 p.m., Jie Yu wrote: 3rdparty/libprocess/src/process.cpp, lines 1149-1151 https://reviews.apache.org/r/31930/diff/2/?file=896916#file896916line1149 Does this comment still apply? Looks like we cannot write empty string. Maybe a CHECK? Ben Mahler wrote: Chatted with Jie, this is the clunkiness that is induced from introducing Optionstring per benh and joris' comments. Callers **still** have to think about the empty string case because the type can express it. The alternative is we add an implicit invariant that empty reads are never returned, which seems pretty hacky (all callers will CHECK against it). Will keep this as a special case for now. But we may want to circle back to Futurestring, as it seems clearer now that we'll probably not want to use a typed StreamT abstraction for this case. Rather, for raw byte streams we probably want to stick with a specialized abstraction (like Pipe). Jie Yu wrote: Chatted with BenM on this. We probably want two abstractions here for streaming. 1) Typed object stream. I.e., StreamT. And there is a 1:1 mapping between reads and writes. 2) Untyped byte stream. I.e., Pipe. No 1:1 mapping between reads and writes (i.e., we can combine writes). For 1), it does makes sense to return an FutureOptionT when read. In fact, we have to because we need a way to tell if it's an end-of-file or an error. For 2), I don't think returning FutureOptionstring is a good idea. The stream is untyped bytes, returning an empty string in the non EOF case shouldn't be possible because it should instead be blocked. Therefore, as benM said, intead of relying on caller to do the CHECK every time, why not just use Futurestring and use empty string to signify the EOF. It looks like [Futurestring Socket::recv()](https://github.com/apache/mesos/blob/0.22.0-rc4/3rdparty/libprocess/include/process/socket.hpp#L87) takes the empty string - EOF approach as well, although it's not documented ;) ;) I'll circle back to Futurestring for now per the original approach. If we want to follow up to use Optionstring we should probably look at the Socket API as well to make these consistent. :) - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/#review76931 --- On March 19, 2015, 9:33 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/ --- (Updated March 19, 2015, 9:33 p.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- See MESOS-2438 for the motivation. Diffs - 3rdparty/libprocess/include/process/http.hpp 10143fdbd5eb43f968c41957a2335f96a097d82e 3rdparty/libprocess/src/http.cpp 7c0cee404645e1dd1a9253f85883aefa29a5f999 3rdparty/libprocess/src/process.cpp e7b029ba97e640c2102548c190ba62b30602f43d 3rdparty/libprocess/src/tests/http_tests.cpp 800752a57230d7768d3d741fef87f6283757e424 Diff: https://reviews.apache.org/r/31930/diff/ Testing --- * added tests * make check Thanks, Ben Mahler
Re: Review Request 31930: Introduced an http::Pipe abstraction to simplify streaming HTTP Responses.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/ --- (Updated March 20, 2015, 6:18 a.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere. Changes --- * Moved back to `Futurestring` to simplify the callers, and to keep things consistent with the Socket API. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- See MESOS-2438 for the motivation. Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp 10143fdbd5eb43f968c41957a2335f96a097d82e 3rdparty/libprocess/src/http.cpp 7c0cee404645e1dd1a9253f85883aefa29a5f999 3rdparty/libprocess/src/process.cpp e7b029ba97e640c2102548c190ba62b30602f43d 3rdparty/libprocess/src/tests/http_tests.cpp 800752a57230d7768d3d741fef87f6283757e424 Diff: https://reviews.apache.org/r/31930/diff/ Testing --- * added tests * make check Thanks, Ben Mahler
Review Request 32338: Moved http encode/decode from header to .cpp file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32338/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Many of the other http definitions are already moved to the .cpp file, this continues that trend. Moving them out makes the API a lot more readable from the scanning the header. Diffs - 3rdparty/libprocess/include/process/http.hpp 2b366986b1f617e95bda94e07f2a0e532f5626f6 3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef Diff: https://reviews.apache.org/r/32338/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 32351: Added http::streaming::get/post for client-side streaming responses.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32351/ --- Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- The caller must make the decision as to whether it expects to stream the response via PIPE or get the full BODY response, otherwise they must implement both cases, which is pretty hacky. So the approach here is to introduce a streaming:: namespace for this. Long term I'd like to just have a http::request(..., Request) function rather than the overloads. Diffs - 3rdparty/libprocess/include/process/http.hpp 2b366986b1f617e95bda94e07f2a0e532f5626f6 3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef 3rdparty/libprocess/src/tests/http_tests.cpp 17fb092a851e128c137152fdce57e9fb10a08bf7 Diff: https://reviews.apache.org/r/32351/diff/ Testing --- Added a test. Thanks, Ben Mahler
Review Request 32342: Updated the http-parser upgrade TODO.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32342/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- http-parser is now hosted under joyent here: https://github.com/joyent/http-parser Diffs - 3rdparty/libprocess/src/decoder.hpp 45f3d7fff2c33ce9c37a8ecbdcbf2c0f02ee76e8 Diff: https://reviews.apache.org/r/32342/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 32345: Removed use of 'assert' in decoder.hpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32345/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Used `CHECK_` family of assertions instead of `assert()`. Diffs - 3rdparty/libprocess/src/decoder.hpp 45f3d7fff2c33ce9c37a8ecbdcbf2c0f02ee76e8 Diff: https://reviews.apache.org/r/32345/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 32337: Moved http::initialize from header to .cpp file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32337/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Many of the other http definitions are already moved to the .cpp file, this continues that trend. Moving them out makes the API a lot more readable from the scanning the header. Diffs - 3rdparty/libprocess/include/process/http.hpp 2b366986b1f617e95bda94e07f2a0e532f5626f6 3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef Diff: https://reviews.apache.org/r/32337/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 32348: Re-ordered and updated documentation for http::get/post/put.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32348/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- No changes to functionality here, just ordering the calls based on the names and making the 'BODY' response explicit in the documentation. Diffs - 3rdparty/libprocess/include/process/http.hpp 2b366986b1f617e95bda94e07f2a0e532f5626f6 3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef Diff: https://reviews.apache.org/r/32348/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 32343: Re-ordered decoder callbacks to be in execution order.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32343/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- This is a pure code movement to order the decoder callbacks in the same order that they will be called by the http_parser. Diffs - 3rdparty/libprocess/src/decoder.hpp 45f3d7fff2c33ce9c37a8ecbdcbf2c0f02ee76e8 Diff: https://reviews.apache.org/r/32343/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 32341: Fixed a copy/paste naming mistake in decoder_tests.cpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32341/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- This mistake came when this test was copied from the request tests. Diffs - 3rdparty/libprocess/src/tests/decoder_tests.cpp d65f5cf25eaecd5994404ccdf07f32d703a5955b Diff: https://reviews.apache.org/r/32341/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 32350: Added a wrapper for HttpProcess to prevent segfaults during test failures.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32350/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- If a test fails, the HttpProcess will be destructed but libprocess still has a handle to it! Diffs - 3rdparty/libprocess/src/tests/http_tests.cpp 17fb092a851e128c137152fdce57e9fb10a08bf7 Diff: https://reviews.apache.org/r/32350/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 32347: Added a StreamingResponseDecoder.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32347/ --- Review request for mesos, Benjamin Hindman and Jie Yu. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- Provides a response decoder that returns 'PIPE' responses once the response headers are received, but before the body data is received. Callers are expected to read the body from the Pipe::Reader in the response. This is needed to receive streaming responses on the client-side. Diffs - 3rdparty/libprocess/src/decoder.hpp 45f3d7fff2c33ce9c37a8ecbdcbf2c0f02ee76e8 3rdparty/libprocess/src/tests/decoder_tests.cpp d65f5cf25eaecd5994404ccdf07f32d703a5955b Diff: https://reviews.apache.org/r/32347/diff/ Testing --- Added a test. Thanks, Ben Mahler
Review Request 32340: Moved http::URL output operator from header to .cpp file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32340/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Many of the other http definitions are already moved to the .cpp file, this continues that trend. Moving them out makes the API a lot more readable from the scanning the header. Diffs - 3rdparty/libprocess/include/process/http.hpp 2b366986b1f617e95bda94e07f2a0e532f5626f6 3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef Diff: https://reviews.apache.org/r/32340/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 32349: Removed http::put and added a TODO.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32349/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- This was not used in the code. I've added a TODO to avoid proliferating functions and parameters here by taking a 'Request' object. Cody has a patch here as well but doesn't have the time to work on it atm: https://reviews.apache.org/r/28254/ Diffs - 3rdparty/libprocess/include/process/http.hpp 2b366986b1f617e95bda94e07f2a0e532f5626f6 3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef Diff: https://reviews.apache.org/r/32349/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 32339: Moved http::path::parse from header to .cpp file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32339/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Many of the other http definitions are already moved to the .cpp file, this continues that trend. Moving them out makes the API a lot more readable from the scanning the header. Diffs - 3rdparty/libprocess/include/process/http.hpp 2b366986b1f617e95bda94e07f2a0e532f5626f6 3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef Diff: https://reviews.apache.org/r/32339/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 32346: Added failure semantics for http::Pipe::Writer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32346/ --- Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- The Pipe writer needs to be able to fail the reader when it cannot complete the pipe. For example, if we are decoding a response and we cannot parse the body, we must ensure the reader of the body sees a failure isntead of EOF. This was used in the subsequent patches in this chain. Diffs - 3rdparty/libprocess/include/process/http.hpp 2b366986b1f617e95bda94e07f2a0e532f5626f6 3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef 3rdparty/libprocess/src/tests/http_tests.cpp 17fb092a851e128c137152fdce57e9fb10a08bf7 Diff: https://reviews.apache.org/r/32346/diff/ Testing --- Added a test. Thanks, Ben Mahler
Review Request 32336: Moved http::Request::accepts from header into cpp file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32336/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Many of the other http definitions are already moved to the .cpp file, this continues that trend. Moving them out makes the API a lot more readable from the scanning the header. Diffs - 3rdparty/libprocess/include/process/http.hpp 2b366986b1f617e95bda94e07f2a0e532f5626f6 3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef Diff: https://reviews.apache.org/r/32336/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 32233: Replaced raw pointer by Owned pointer
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32233/#review77259 --- Ship it! Ship It! - Ben Mahler On March 20, 2015, 8:11 a.m., Akanksha Agrawal wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32233/ --- (Updated March 20, 2015, 8:11 a.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- Replaced raw pointer by Owned pointer Diffs - src/slave/containerizer/external_containerizer.hpp 70491375a6cd8988a33e3f18870c9170a37c0f17 src/slave/containerizer/external_containerizer.cpp 42c67f548caf7bddbe131e0dfa7d74227d8c2593 Diff: https://reviews.apache.org/r/32233/diff/ Testing --- Thanks, Akanksha Agrawal
Re: Review Request 28254: http: Allow sending Request objects
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28254/#review77073 --- Interesting change cody? Are you still looking to get this in? I'm hoping we can clean up the current design of http::get and http::post, where the function arguments have essentially grown to be subsets of things inside the 'Request' object. This seems like a first step towards that, what was your plan for http::get and http::post? - Ben Mahler On Nov. 20, 2014, 12:54 a.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28254/ --- (Updated Nov. 20, 2014, 12:54 a.m.) Review request for mesos. Bugs: MESOS-2132 https://issues.apache.org/jira/browse/MESOS-2132 Repository: mesos Description --- libprocess gives HTTP Handlers a Request object. If one of those handlers wants to act as a proxy and forward all/part of that request across the network, it currently has to split up the object it is given, manually encode the query string, and then call either http::get() or http::post() (Other HTTP methods aren't supported). Adds a general mechanism for forwarding requests to remote nodes (IP/Port) pairs and getting back the response. Cleans up some right angle brackets as a drive-by fix. Diffs - 3rdparty/libprocess/include/process/http.hpp 9cf05acbb724ab9af8010d1788621d37a0e48e86 3rdparty/libprocess/src/http.cpp b00f9366f5c06b6f20e38c5ae0c23b8a9358 Diff: https://reviews.apache.org/r/28254/diff/ Testing --- make distcheck ubuntu 14.04 Thanks, Cody Maloney
Re: Review Request 31930: Introduced an http::Pipe abstraction to simplify streaming HTTP Responses.
On March 18, 2015, 11:49 p.m., Jie Yu wrote:
3rdparty/libprocess/include/process/http.hpp, lines 167-186
https://reviews.apache.org/r/31930/diff/2/?file=896914#file896914line167
Instead of keeping a reference to Data, wouldn't be simpler and more
straitforward to keep a Pipe instance (as Pipe is already copyable) here?
You can avoid the forward declarations as well.
```
class Pipe
{
public:
class Reader
{
public:
...
private:
// Necessary for accessing pipe.data.
friend class Pipe;
Reader(const Pipe _pipe) : pipe(_pipe) {}
Pipe pipe;
};
};
```
Chatted with Jie, this leads to an incomplete type issue during compilation and
it's fairly clunky to work around it (need to forward declare Reader/Writer in
Pipe, and declare Reader/Writer fully outside Pipe).
On March 18, 2015, 11:49 p.m., Jie Yu wrote:
3rdparty/libprocess/include/process/http.hpp, line 205
https://reviews.apache.org/r/31930/diff/2/?file=896914#file896914line205
Are you exposing this function only for testing? If yes, please add a
comment.
Chatted with Jie, this is necessary for the pipe writer to detect a
disconnection (e.g. master streaming events to a framework, framework
disconnects).
On March 18, 2015, 11:49 p.m., Jie Yu wrote:
3rdparty/libprocess/include/process/http.hpp, lines 208-209
https://reviews.apache.org/r/31930/diff/2/?file=896914#file896914line208
Ditto.
Per above we'll store Data to avoid the incomplete type issues.
On March 18, 2015, 11:49 p.m., Jie Yu wrote:
3rdparty/libprocess/src/http.cpp, lines 44-46
https://reviews.apache.org/r/31930/diff/2/?file=896915#file896915line44
This can be simplified as:
```
return Pipe::Reader(*this);
```
I've added a constructor to avoid the default initialization issue, thanks!
On March 18, 2015, 11:49 p.m., Jie Yu wrote:
3rdparty/libprocess/src/http.cpp, line 101
https://reviews.apache.org/r/31930/diff/2/?file=896915#file896915line101
Can you add an extra parathesis here. I am not super familar with the
operator priorities:)
Jie and I chatted about this, we probably wouldn't add parens if this was +, ,
!=, etc, and we already have this without parens in other places in the code.
On March 18, 2015, 11:49 p.m., Jie Yu wrote:
3rdparty/libprocess/src/process.cpp, lines 1149-1151
https://reviews.apache.org/r/31930/diff/2/?file=896916#file896916line1149
Does this comment still apply? Looks like we cannot write empty string.
Maybe a CHECK?
Chatted with Jie, this is the clunkiness that is induced from introducing
Optionstring per benh and joris' comments.
Callers **still** have to think about the empty string case because the type
can express it. The alternative is we add an implicit invariant that empty
reads are never returned, which seems pretty hacky (all callers will CHECK
against it).
Will keep this as a special case for now. But we may want to circle back to
Futurestring, as it seems clearer now that we'll probably not want to use a
typed StreamT abstraction for this case. Rather, for raw byte streams we
probably want to stick with a specialized abstraction (like Pipe).
- Ben
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31930/#review76931
---
On March 16, 2015, 11:49 p.m., Ben Mahler wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31930/
---
(Updated March 16, 2015, 11:49 p.m.)
Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere.
Bugs: MESOS-2438
https://issues.apache.org/jira/browse/MESOS-2438
Repository: mesos
Description
---
See MESOS-2438 for the motivation.
Diffs
-
3rdparty/libprocess/include/process/http.hpp
10143fdbd5eb43f968c41957a2335f96a097d82e
3rdparty/libprocess/src/http.cpp 7c0cee404645e1dd1a9253f85883aefa29a5f999
3rdparty/libprocess/src/process.cpp
e7b029ba97e640c2102548c190ba62b30602f43d
3rdparty/libprocess/src/tests/http_tests.cpp
800752a57230d7768d3d741fef87f6283757e424
Diff: https://reviews.apache.org/r/31930/diff/
Testing
---
* added tests
* make check
Thanks,
Ben Mahler
Re: Review Request 31930: Introduced an http::Pipe abstraction to simplify streaming HTTP Responses.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/ --- (Updated March 19, 2015, 9:33 p.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere. Changes --- Changes from Jie's review: * Prevent default construction of `Pipe::Reader`s and `Pipe::Writer`s. * Make the `Response` '`reader`' an `Option`. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- See MESOS-2438 for the motivation. Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp 10143fdbd5eb43f968c41957a2335f96a097d82e 3rdparty/libprocess/src/http.cpp 7c0cee404645e1dd1a9253f85883aefa29a5f999 3rdparty/libprocess/src/process.cpp e7b029ba97e640c2102548c190ba62b30602f43d 3rdparty/libprocess/src/tests/http_tests.cpp 800752a57230d7768d3d741fef87f6283757e424 Diff: https://reviews.apache.org/r/31930/diff/ Testing --- * added tests * make check Thanks, Ben Mahler
Re: Review Request 31930: Introduced an http::Pipe abstraction to simplify streaming HTTP Responses.
On March 13, 2015, 4:46 a.m., Benjamin Hindman wrote: 3rdparty/libprocess/include/process/http.hpp, line 145 https://reviews.apache.org/r/31930/diff/1/?file=891209#file891209line145 Also explain that this is an in-memory single OS process pipe, and is not backed by file descriptors so can't be used across a fork. It says in-memory near the very top. I'll add a sentence after that one to call out that this cannot be used across OS processes, hopefully that further avoids possible confusion. On March 13, 2015, 4:46 a.m., Benjamin Hindman wrote: 3rdparty/libprocess/include/process/http.hpp, line 130 https://reviews.apache.org/r/31930/diff/1/?file=891209#file891209line130 s/streaming HTTP data/sending an HTTP response via the chunked transfer encoding/ Such a comment is likely to change very soon, when we want to receive responses in a streaming manner. But I'll add it to be more specific for now. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/#review76236 --- On March 11, 2015, 7:41 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/ --- (Updated March 11, 2015, 7:41 a.m.) Review request for mesos, Benjamin Hindman and Jie Yu. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- See MESOS-2438 for the motivation. Diffs - 3rdparty/libprocess/include/process/http.hpp 10143fdbd5eb43f968c41957a2335f96a097d82e 3rdparty/libprocess/src/http.cpp 7c0cee404645e1dd1a9253f85883aefa29a5f999 3rdparty/libprocess/src/process.cpp e7b029ba97e640c2102548c190ba62b30602f43d 3rdparty/libprocess/src/tests/http_tests.cpp 800752a57230d7768d3d741fef87f6283757e424 Diff: https://reviews.apache.org/r/31930/diff/ Testing --- * added tests * make check Thanks, Ben Mahler
Re: Review Request 31930: Introduced an http::Pipe abstraction to simplify streaming HTTP Responses.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/ --- (Updated March 16, 2015, 11:49 p.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere. Changes --- * Added joris as a reviewer. * Made end-of-file more explicit through an Option. * Renamed '`promises`' to '`reads`' and '`items`' to '`writes`'. * Added additional comments. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- See MESOS-2438 for the motivation. Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp 10143fdbd5eb43f968c41957a2335f96a097d82e 3rdparty/libprocess/src/http.cpp 7c0cee404645e1dd1a9253f85883aefa29a5f999 3rdparty/libprocess/src/process.cpp e7b029ba97e640c2102548c190ba62b30602f43d 3rdparty/libprocess/src/tests/http_tests.cpp 800752a57230d7768d3d741fef87f6283757e424 Diff: https://reviews.apache.org/r/31930/diff/ Testing --- * added tests * make check Thanks, Ben Mahler
Re: Review Request 31930: Introduced an http::Pipe abstraction to simplify streaming HTTP Responses.
Can we add a comment about the 2 usage patterns for 'read()': 1) Call 'read()' one at a time. This guarantees order of data vs EOF vs closed. 2) Make multiple 'read()' calls while waiting on multiple futures. This has no order guarantees due to the races between the locked and callback invocation sections. (see comments below) Great eye Joris! I also noticed this when initially making this change as it's a property of process::Queue, see the note I left here: https://reviews.apache.org/r/31788/diff/1/#1 However, after having chatted with Jie at the time, and having thought through this further, this is expected for a few reasons: * Our current callback model on Future is to invoke callbacks synchronously, however we should not be allowing callers to assume this is the case! For example, we even had a TODO [here](https://github.com/apache/mesos/blob/0.21.0/3rdparty/libprocess/include/process/future.hpp#L1591) to invoke callbacks from another execution context. Unfortunately that TODO was wiped out in your change [here](https://reviews.apache.org/r/28195) :) I just restored that TODO to make it clear that we don't want to be guaranteeing synchronous callback execution. * Even if we were setting these in the critical section, the callers may not have added the callbacks on the returned Futures yet! (if they context switch after calling read but before calling e.g. `.onAny()`) This means we still would not be maintaining ordering of the callback invocation, since we do not control when the callbacks are added, the callers do. I think we want to avoid specifying this kind of behavior on http::Pipe, since it's inherent to libprocess and our asychronous model. Also, we're not planning to use http::Pipe concurrently like this. On March 13, 2015, 1:25 a.m., Joris Van Remoortere wrote: 3rdparty/libprocess/include/process/http.hpp, lines 129-145 https://reviews.apache.org/r/31930/diff/1/?file=891209#file891209line129 Can we add a high-level comment about the intended usage pattern? -We can have multiple readers mapped to the same pipe, and the data is shared between them. Once one reader calls 'close()', all the readers' futures are completed with EOFs... Once one reader calls 'close()', all the readers' futures are completed with EOFs... Do you mean when the writer calls close and all data has been read? That's when the reads return EOF, per the comment above (much like unix pipes ...). Per our discussion below, I'd like to avoid documenting ineherent concurrency properties of libprocess here. We're planning to only use this with one reader and one writer, much like process::Queue. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/#review76268 --- On March 11, 2015, 7:41 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/ --- (Updated March 11, 2015, 7:41 a.m.) Review request for mesos, Benjamin Hindman and Jie Yu. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- See MESOS-2438 for the motivation. Diffs - 3rdparty/libprocess/include/process/http.hpp 10143fdbd5eb43f968c41957a2335f96a097d82e 3rdparty/libprocess/src/http.cpp 7c0cee404645e1dd1a9253f85883aefa29a5f999 3rdparty/libprocess/src/process.cpp e7b029ba97e640c2102548c190ba62b30602f43d 3rdparty/libprocess/src/tests/http_tests.cpp 800752a57230d7768d3d741fef87f6283757e424 Diff: https://reviews.apache.org/r/31930/diff/ Testing --- * added tests * make check Thanks, Ben Mahler
Re: Review Request 31677: stout: Make the counting of netmask set bits more efficient.
On March 11, 2015, 7 p.m., Ben Mahler wrote: I'm curious, what prompted this change? Dominic Hamon wrote: a comment on the original version in a review that it wasn't the best way of counting bits. it seems like a generally useful thing to do. Evelina Dumitrescu wrote: another benefit: IPv6 netmasks, where there are 128 bits. I don't disagree with this change at all. I'm just hoping someone actually put some thought into why this matters: Are we optimizing with an understanding of where the bottlenecks are, and how this avoids them? For example, when is this bit counting occurring? How often? Is it in a critical path of the code? These are the kinds of questions its very important to be asking ourselves when we decide to optimize things, otherwise we start to introduce complexity in the wrong places. On March 11, 2015, 7 p.m., Ben Mahler wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/bits.hpp, line 25 https://reviews.apache.org/r/31677/diff/5/?file=890606#file890606line25 Can we return a size_t to capture that this is a count and that negatives are not possible? Is `bits::count` not a sufficient name? Dominic Hamon wrote: as per the style guide, http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types, You should not use the unsigned integer types such as uint32_t, unless there is a valid reason such as representing a bit pattern rather than a number, or you need defined overflow modulo 2^N. In particular, do not use unsigned types to say a number will never be negative. Instead, use assertions for this. there may be a countUnsetBits in the future. Maybe countSet, but we could bikeshed all day :) From that same section... *When appropriate, you are welcome to use standard types like **size_t** and ptrdiff_t.* A grep through the code shows we use size_t heavily. In the case of size_t, we're using the type system to provide valuable information to the caller. The caller understands that a size is being returned and does not need to think about what it means for this to return -1. I'd like to understand *why* you think we shouldn't use size_t since we're already leveraging it heavily. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31677/#review76105 --- On March 10, 2015, 7:59 p.m., Evelina Dumitrescu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31677/ --- (Updated March 10, 2015, 7:59 p.m.) Review request for mesos, Benjamin Hindman, Dominic Hamon, Jie Yu, Joris Van Remoortere, and Niklas Nielsen. Repository: mesos Description --- see summary Diffs - 3rdparty/libprocess/3rdparty/stout/Makefile.am a5224554f6851930aa97cadc5da3d010829d87dc 3rdparty/libprocess/3rdparty/stout/include/Makefile.am ac2bbed6fe86623fb51cac3613d79d7b1372df9d 3rdparty/libprocess/3rdparty/stout/include/stout/bits.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp e4e86deb8a42a1d91f4d4ac210eae26f8f57ee17 3rdparty/libprocess/3rdparty/stout/tests/bits_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/31677/diff/ Testing --- Thanks, Evelina Dumitrescu
Re: Review Request 32001: Required a period in trailing comments in the style guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32001/#review76286 --- docs/mesos-c++-style-guide.md https://reviews.apache.org/r/32001/#comment123806 Hm, isn't this captured by ending each sentence with a period above? - Ben Mahler On March 12, 2015, 8:59 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32001/ --- (Updated March 12, 2015, 8:59 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary. Diffs - docs/mesos-c++-style-guide.md 439fe12ad137c1bed3a21d5a097c60a48f10a4f9 Diff: https://reviews.apache.org/r/32001/diff/ Testing --- None (not a functional change). Thanks, Alexander Rukletsov
Re: Review Request 31992: Added a TODO for readability reviews.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31992/#review76271 --- * Braces: placed on separate line for class / struct / function definitions. Same line otherwise. Do we want to tackle higher level style here that is currently missing from the style guide? * Use Try / Result to describe functions that can fail synchronously (rather than returning NULL, -1, etc). * Use Option to capture something that is optionally set (rather than NULL, -1, etc). * Use Future failure to capture asychronous failures. * Do not block inside the execution context of a libprocess Process. - Ben Mahler On March 12, 2015, 3:59 p.m., Benjamin Hindman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31992/ --- (Updated March 12, 2015, 3:59 p.m.) Review request for mesos, Bernd Mathiske and Ben Mahler. Repository: mesos Description --- See summary. Diffs - readability/TODO PRE-CREATION Diff: https://reviews.apache.org/r/31992/diff/ Testing --- N/A Thanks, Benjamin Hindman
Re: Review Request 31988: Do not have search filter input field disappear when query yields no results
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31988/#review76270 --- I'd love to help land this for you. Would you mind describing how this fixes the issue, how you tested this, and include screenshots if possible? Thank you! - Ben Mahler On March 12, 2015, 2:18 p.m., Joe Lee wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31988/ --- (Updated March 12, 2015, 2:18 p.m.) Review request for mesos. Bugs: MESOS-2479 https://issues.apache.org/jira/browse/MESOS-2479 Repository: mesos Description --- Do not have search filter input field disappear when query yields no results Diffs - src/webui/master/static/directives/tableHeader.html ad64864c0ab76165286c4f05a7af1245d7b09196 Diff: https://reviews.apache.org/r/31988/diff/ Testing --- Thanks, Joe Lee
Re: Review Request 31700: Reserved memory for JSON arrays where appropriate.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31700/#review76276 --- Ship it! Thanks Alex! Let's look at using std::move as well if that works on gcc 4.4 (since that's still inside configure.ac). src/common/http.cpp https://reviews.apache.org/r/31700/#comment123796 Let's end all of these comments with a period. I'll take care of this before committing. - Ben Mahler On March 11, 2015, 10:40 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31700/ --- (Updated March 11, 2015, 10:40 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2353 https://issues.apache.org/jira/browse/MESOS-2353 Repository: mesos Description --- See summary. Diffs - src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9 src/master/http.cpp 0b56cb419b0e488eb4163739f57ee1837ca83d24 Diff: https://reviews.apache.org/r/31700/diff/ Testing --- make check (OS X 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 31700: Reserved memory for JSON arrays where appropriate.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/#review76133
---
Ship it!
Thanks alex, just one minor item and we can get this committed!
src/common/http.cpp
https://reviews.apache.org/r/31700/#comment123581
Can you make these consistent with the ones below?
E.g.
```
{
JSON::Array array;
array.reserve(task.statuses().size()); // MESOS-2353.
foreach (const TaskStatus status, task.statuses()) {
statuses.values.push_back(model(status));
}
object.values[statuses] = array;
}
{
JSON::Array array;
array.reserve(task.labels().labels().size()); // MESOS-2353.
foreach (const Label label, task.labels().labels()) {
labels.values.push_back(JSON::Protobuf(label));
}
object.values[labels] = array;
}
```
src/master/http.cpp
https://reviews.apache.org/r/31700/#comment123582
Do you know if a move is helpful here or if the compiler is already
optimizing this?
```
object.values[slaves] = std::move(array);
```
Don't do it in this change, we need to measure and I'm just curious :)
- Ben Mahler
On March 7, 2015, 1:43 a.m., Alexander Rukletsov wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31700/
---
(Updated March 7, 2015, 1:43 a.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-2353
https://issues.apache.org/jira/browse/MESOS-2353
Repository: mesos
Description
---
See summary.
Diffs
-
src/common/http.cpp 0b57fb01f7769031704e5341849bf95a0197ffd9
src/master/http.cpp b8eef69505b147d4c8a0e005dff545b9fc12a220
Diff: https://reviews.apache.org/r/31700/diff/
Testing
---
make check (OS X 10.9.5, Ubuntu 14.04)
Thanks,
Alexander Rukletsov
Re: Review Request 31873: Added Java binding for the new acceptOffers API.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31873/#review76106 --- Ship it! This patch looks good, but can you please include updates to the CHANGELOG and Upgrades documents in this review chain to capture this API change in 0.23.0? src/examples/java/TestFramework.java https://reviews.apache.org/r/31873/#comment123515 Do you need to fix this in this patch? Just commit the whitespace removal on master if you really want it. src/examples/java/TestFramework.java https://reviews.apache.org/r/31873/#comment123516 Can we move '`launch`' up and use it to store our tasks, instead of using the extra `ListTaskInfo`? src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp https://reviews.apache.org/r/31873/#comment123519 Want to collapse these lines to just `push_back(construct...(...));` src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp https://reviews.apache.org/r/31873/#comment123520 Want to collapse these lines to just `push_back(construct...(...))`? - Ben Mahler On March 10, 2015, 12:07 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31873/ --- (Updated March 10, 2015, 12:07 a.m.) Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone. Bugs: MESOS-2427 https://issues.apache.org/jira/browse/MESOS-2427 Repository: mesos Description --- Added Java binding for the new acceptOffers API. Diffs - src/examples/java/TestFramework.java 65ba9d9e242f40d6382cd9c18269b87f8b59b5a1 src/java/jni/construct.cpp e54c11ef0ce3e9f6d0e572d3a89cadae417f9cd6 src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 4f0dad78f7300b7bbf147f231eb053fd7faf4b69 src/java/src/org/apache/mesos/MesosSchedulerDriver.java a1055a5d907133485891ebd6d8731c102b913fec src/java/src/org/apache/mesos/SchedulerDriver.java d5b100a4c371bd9c496b9127767c14047185e5f9 Diff: https://reviews.apache.org/r/31873/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 31903: Added Python binding for the acceptOffers API.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31903/#review76110 --- Ship it! This patch looks good, but can you please include updates to the CHANGELOG and Upgrades documents in this review chain to capture this API change in 0.23.0? src/python/native/src/mesos/native/mesos_scheduler_driver_impl.cpp https://reviews.apache.org/r/31903/#comment123529 Can we get some newlines here? src/python/native/src/mesos/native/mesos_scheduler_driver_impl.cpp https://reviews.apache.org/r/31903/#comment123531 newline here? src/python/native/src/mesos/native/mesos_scheduler_driver_impl.cpp https://reviews.apache.org/r/31903/#comment123530 Can we get some newlines here? src/python/native/src/mesos/native/mesos_scheduler_driver_impl.cpp https://reviews.apache.org/r/31903/#comment123532 newline here? - Ben Mahler On March 10, 2015, 6:09 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31903/ --- (Updated March 10, 2015, 6:09 p.m.) Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone. Bugs: MESOS-2428 https://issues.apache.org/jira/browse/MESOS-2428 Repository: mesos Description --- Added Python binding for the acceptOffers API. Diffs - src/examples/python/test_framework.py 27106147900f8b5dd2ea0443f5658902ff1145e4 src/python/interface/src/mesos/interface/__init__.py f3d96a455dc8b66fc4527af1b3dee2f8841b29dd src/python/native/src/mesos/native/mesos_scheduler_driver_impl.hpp a6980002202b829b40e85188bfb291e5d22c9a26 src/python/native/src/mesos/native/mesos_scheduler_driver_impl.cpp bb1884597731c73f4815069ceb940cf067790670 Diff: https://reviews.apache.org/r/31903/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 31677: stout: Make the counting of netmask set bits more efficient.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31677/#review76105 --- I'm curious, what prompted this change? 3rdparty/libprocess/3rdparty/stout/include/stout/bits.hpp https://reviews.apache.org/r/31677/#comment123510 Can we return a size_t to capture that this is a count and that negatives are not possible? Is `bits::count` not a sufficient name? - Ben Mahler On March 10, 2015, 7:59 p.m., Evelina Dumitrescu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31677/ --- (Updated March 10, 2015, 7:59 p.m.) Review request for mesos, Benjamin Hindman, Dominic Hamon, Jie Yu, Joris Van Remoortere, and Niklas Nielsen. Repository: mesos Description --- see summary Diffs - 3rdparty/libprocess/3rdparty/stout/Makefile.am a5224554f6851930aa97cadc5da3d010829d87dc 3rdparty/libprocess/3rdparty/stout/include/Makefile.am ac2bbed6fe86623fb51cac3613d79d7b1372df9d 3rdparty/libprocess/3rdparty/stout/include/stout/bits.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp e4e86deb8a42a1d91f4d4ac210eae26f8f57ee17 3rdparty/libprocess/3rdparty/stout/tests/bits_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/31677/diff/ Testing --- Thanks, Evelina Dumitrescu
Review Request 31930: Introduced an http::Pipe abstraction to simplify streaming HTTP Responses.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/ --- Review request for mesos, Benjamin Hindman and Jie Yu. Bugs: MESOS-2438 https://issues.apache.org/jira/browse/MESOS-2438 Repository: mesos Description --- See MESOS-2438 for the motivation. Diffs - 3rdparty/libprocess/include/process/http.hpp 10143fdbd5eb43f968c41957a2335f96a097d82e 3rdparty/libprocess/src/http.cpp 7c0cee404645e1dd1a9253f85883aefa29a5f999 3rdparty/libprocess/src/process.cpp e7b029ba97e640c2102548c190ba62b30602f43d 3rdparty/libprocess/src/tests/http_tests.cpp 800752a57230d7768d3d741fef87f6283757e424 Diff: https://reviews.apache.org/r/31930/diff/ Testing --- * added tests * make check Thanks, Ben Mahler
Re: Review Request 31664: Added operator+= and operator+ for hashmapSlaveID, Resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31664/#review75571 --- src/common/resources.cpp https://reviews.apache.org/r/31664/#comment122759 This looks inconsistent with all of the other + operator implementations in this file, no? - Ben Mahler On March 6, 2015, 6:46 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31664/ --- (Updated March 6, 2015, 6:46 p.m.) Review request for mesos, Alexander Rukletsov and Ben Mahler. Bugs: MESOS-2373 https://issues.apache.org/jira/browse/MESOS-2373 Repository: mesos Description --- Convenience functions to help aggregate `hashmapSlaveID, Resources`. Used in [r31665](https://reviews.apache.org/r/31665). Diffs - include/mesos/resources.hpp da6d48871a0061d8bbf5e681dd6e7edac659d812 src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 Diff: https://reviews.apache.org/r/31664/diff/ Testing --- make check Thanks, Michael Park
