Re: Review Request 34361: converted hard-coded strings to consts
On June 10, 2015, 1:25 a.m., Ben Mahler wrote: src/tests/master_tests.cpp, lines 3031-3034 https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031 Why bother with all this? Why not just have `key1`, `value1`, `key2`, `value2` inlined appropriately throughout these tests. Very straightforward to read. Colin Williams wrote: I think the issue with the changes remaining is that the test depends on the same value occurring in several places. By consolidating to a variable it's no longer possible for these values to get out of sync. Niklas Nielsen wrote: Colin: exactly Ben: We have label tests three places; in the master, slave and in the modules and they use the same foo, bar, baz, qux key value pairs. The idea was to centralize them, so they don't get out of date and to avoid code duplication. Does that make sense? Ben Mahler wrote: Well, then let's just keep it simple and get rid of these special strings by just making the tests use key1, value1, key2, value2, etc. I'm not following your code duplication point, this introduces more code (now there are additional global constants, which we like to avoid if unnecessary). Also, each test is ideally independent, why does the master's test for labels affect the slave's test for labels? Let's say I need a test with 5 labels, you want to have 5x2=10 global constants to address this? Niklas Nielsen wrote: Try to see how testLabelKey and testLabelValue was used previously and foo, bar, qux was used on others and I created this ticket to unify the way we do this, along with seeing these key value pair being created in the slave and master tests. Dispite the current patch, I do think we can simply and unify the test label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious which key pairs are being tested. The comments in the test code need to carry a bunch of context, to make sense out of the label transformations (especially across the library border for the hooks tests). This is all in spirit of reducing the tech debt we introduced. We are on the same page on not introducing more lines/key pairs than before. Let us iterate on this and get back to you. Colin Williams wrote: Ben: I'm more in agreement with your sentiment, I'm not sure I see the point of unifying label names into a centralized variable that aren't at all related. Niklas: I was a little confused by the original task description, so I thought a sample patch would be a good discussion point. I don't entirely understand the tech debt you're trying to solve. For example, it seems like you're asking to have some constants defined somewhere that are used by both master_test and slave_test, but the the similarities between these two only seem incidental. I'm concerned that creating something like CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code. Niklas Nielsen wrote: Okay - thinking about this; I am on board with key1, value1 etc. Colin: the tech debt is that we have some inlined constants (like foo, bar, etc) and some which are constants (which have to be kept in sync between hooks modules and test body). The idea was to unify the way we name the key value pairs. Do you want to update this ticket to reflect this, or come with a new patch set which tackles streaming the two? Niklas Nielsen wrote: Ping :) Colin Williams wrote: Sorry, my day job's been really all-consuming the last few days. I was planning to update the ticket. Colin Williams wrote: Ticket updated Niklas Nielsen wrote: Colin; thanks! should we keep this patch then, or will you be providing a new one? I'm not planning on providing a new one. - Colin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34361/#review87331 --- On June 8, 2015, 7:05 p.m., Colin Williams wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34361/ --- (Updated June 8, 2015, 7:05 p.m.) Review request for mesos and Niklas Nielsen. Bugs: MESOS-2637 https://issues.apache.org/jira/browse/MESOS-2637 Repository: mesos Description --- converted hard-coded strings to consts Diffs - src/tests/hook_tests.cpp 3ffde6d src/tests/master_tests.cpp ba3858f src/tests/slave_tests.cpp acae497 Diff: https://reviews.apache.org/r/34361/diff/ Testing --- Thanks, Colin Williams
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review91909 --- Patch looks great! Reviews applied: [36547] All tests passed. - Mesos ReviewBot On July 16, 2015, 4:55 p.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 16, 2015, 4:55 p.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36412: Adjusted the revocable cpu isolator test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36412/#review91911 --- Ship it! Ship It! - Vinod Kone On July 11, 2015, 12:09 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36412/ --- (Updated July 11, 2015, 12:09 a.m.) Review request for mesos, Ian Downes and Vinod Kone. Bugs: MESOS-2652 https://issues.apache.org/jira/browse/MESOS-2652 Repository: mesos Description --- Adjusted the revocable cpu isolator test. Diffs - src/tests/isolator_tests.cpp 8eb732fe90dbab11967e663a5c06890f79463488 Diff: https://reviews.apache.org/r/36412/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 36411: Used low cpu.shares for revocable containers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36411/#review91912 --- Ship it! Ship It! - Vinod Kone On July 11, 2015, 12:09 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36411/ --- (Updated July 11, 2015, 12:09 a.m.) Review request for mesos, Ian Downes and Vinod Kone. Bugs: MESOS-2652 https://issues.apache.org/jira/browse/MESOS-2652 Repository: mesos Description --- Used low cpu.shares for revocable containers. Diffs - src/slave/containerizer/isolators/cgroups/constants.hpp e6df4a29e9af8076d6454740afa61fce04c3d06b src/slave/containerizer/isolators/cgroups/cpushare.cpp f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 Diff: https://reviews.apache.org/r/36411/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 34137: Add support for container image provisioners.
On July 8, 2015, 11:34 p.m., Vinod Kone wrote: src/slave/containerizer/mesos/containerizer.cpp, line 418 https://reviews.apache.org/r/34137/diff/2/?file=989756#file989756line418 Is it possible for rootfs to not exist when we are here? If not, there should be a CHECK and a comment on what guarantees its presence (the fact that there is a forked pid?). the field rootfs is just a option, so I think that's why you can just pass that in. On July 8, 2015, 11:34 p.m., Vinod Kone wrote: include/mesos/slave/isolator.hpp, lines 70-71 https://reviews.apache.org/r/34137/diff/2/?file=989753#file989753line70 We don't align arguments for constructors this way IIRC. ExecutorRunState(arg1, arg2, ); Looks like the latest revision has the right format right? On July 8, 2015, 11:34 p.m., Vinod Kone wrote: src/slave/containerizer/mesos/containerizer.cpp, line 1249 https://reviews.apache.org/r/34137/diff/2/?file=989756#file989756line1249 why include the containerid? doesn't the caller of this method already know that? The caller knows this, but since this shows up in the log it's easier to correlate the error with a container id. On July 8, 2015, 11:34 p.m., Vinod Kone wrote: src/slave/containerizer/mesos/containerizer.cpp, line 1280 https://reviews.apache.org/r/34137/diff/2/?file=989756#file989756line1280 ditto. why include the container id? You're right I don't tihnk containerId here is needed. On July 8, 2015, 11:34 p.m., Vinod Kone wrote: src/slave/containerizer/mesos/containerizer.cpp, line 818 https://reviews.apache.org/r/34137/diff/2/?file=989756#file989756line818 where is the update to launchFlags to add 'rootfs' ? which review? Looks like this field is already added in master. commit 15bf9f67e3d9489e49b2176e1e4221a1a47fd0c9 Author: Ian Downes idow...@twitter.com Date: Thu Dec 18 15:46:15 2014 -0800 Support chrooting in MesosContainerizer launch helper. Review: https://reviews.apache.org/r/31444/ - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review91014 --- On July 12, 2015, 4:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 12, 2015, 4:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36412: Adjusted the revocable cpu isolator test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36412/#review91914 --- src/tests/isolator_tests.cpp (line 417) https://reviews.apache.org/r/36412/#comment145630 I would recommend to keep the IDLE check because that code still exists. - Vinod Kone On July 11, 2015, 12:09 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36412/ --- (Updated July 11, 2015, 12:09 a.m.) Review request for mesos, Ian Downes and Vinod Kone. Bugs: MESOS-2652 https://issues.apache.org/jira/browse/MESOS-2652 Repository: mesos Description --- Adjusted the revocable cpu isolator test. Diffs - src/tests/isolator_tests.cpp 8eb732fe90dbab11967e663a5c06890f79463488 Diff: https://reviews.apache.org/r/36412/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 36413: Removed the code of setting SCHED_IDLE policy for revocable containers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36413/#review91915 --- Ship it! src/slave/containerizer/isolators/cgroups/cpushare.cpp https://reviews.apache.org/r/36413/#comment145632 I see. So you removed it here. Can you remove it from the test in this review instead of the previous one? - Vinod Kone On July 11, 2015, 12:09 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36413/ --- (Updated July 11, 2015, 12:09 a.m.) Review request for mesos, Ian Downes and Vinod Kone. Bugs: MESOS-2652 https://issues.apache.org/jira/browse/MESOS-2652 Repository: mesos Description --- Removed the code of setting SCHED_IDLE policy for revocable containers. Diffs - src/slave/containerizer/isolators/cgroups/cpushare.cpp f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 Diff: https://reviews.apache.org/r/36413/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 34137: Add support for container image provisioners.
On July 14, 2015, 7:41 p.m., Vinod Kone wrote: src/slave/containerizer/provisioner.cpp, lines 43-56 https://reviews.apache.org/r/34137/diff/2-3/?file=989758#file989758line43 Why do you need foreach loop here if you were going to return error anyway? Timothy Chen wrote: We need the foreach to go over all the provisioners though, as there could be more than one although there is one listed for now. I can remove the for loop and just return empty map, and also add a log that provisioners are not supported yet if something is specified. Sounds good? - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review91662 --- On July 12, 2015, 4:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 12, 2015, 4:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review91913 --- Ship it! src/launcher/fetcher.cpp (line 134) https://reviews.apache.org/r/36547/#comment145628 I assume those are spaces? Could you please doublecheck? src/launcher/fetcher.cpp (line 136) https://reviews.apache.org/r/36547/#comment145626 can you add a short comment why we assume this here? - Joerg Schad On July 16, 2015, 4:55 p.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 16, 2015, 4:55 p.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 34136: Add ContainerImage protobuf.
On July 14, 2015, 9:03 p.m., Jiang Yan Xu wrote: include/mesos/mesos.proto, lines 1211-1213 https://reviews.apache.org/r/34136/diff/3/?file=1009139#file1009139line1211 So I found the use of the field `id` inconsistent in the code. Sometimes `id` has the `sha512-` prefix and sometimes not. I think we should consistently refer to `id` using the definition in the [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id), i.e., with the prefix. The fact that the ID is computed by the image creator using sha512 and that the provisioner validates it using sha512 is merely an implementation detail that is not a conern of higher level abstractions / APIs. So here I think in the comments we should not call it Image hash but rather refer to the spec for its full definition. We can of course call out the fact that it should have the sha512- perfix. What do you think? Hi there, I'm going to commit this for Ian and just saw your comment. How about I reword the comment here to // The ID of the Image. Please refer to the Appc spec for its definition.? - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34136/#review91666 --- On July 12, 2015, 4:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34136/ --- (Updated July 12, 2015, 4:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Add ContainerImage protobuf. Diffs - include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 Diff: https://reviews.apache.org/r/34136/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36518: Fixed a bug in master to properly handle resubscription.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36518/ --- (Updated July 16, 2015, 9:48 p.m.) Review request for mesos and Ben Mahler. Changes --- benm's comments. NNFR. Bugs: MESOS-3055 https://issues.apache.org/jira/browse/MESOS-3055 Repository: mesos Description --- In the process of fixing this, added an additional check in Master::registerFramework() that should've been there in the first place. Similar check exists in Master::reregisterFramework(). Diffs (updated) - src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 Diff: https://reviews.apache.org/r/36518/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36518: Fixed a bug in master to properly handle resubscription.
On July 15, 2015, 9:38 p.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, line 143 https://reviews.apache.org/r/36518/diff/1/?file=1012851#file1012851line143 Hm.. 'force' doesn't make sense for SUBSCRIBE without a framework id. Seems like either we: (1) Make 'force' optional, and document that it is only relevant when the framework id is set (re-subscription). (2) Remove 'force' from SUBSCRIBE, add a RESUBSCRIBE with 'force'? =/ I'll make 'force' optional in a dependent different review. On July 15, 2015, 9:38 p.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, line 163 https://reviews.apache.org/r/36518/diff/1/?file=1012851#file1012851line163 why the newline here but not above? looked dense. will remove. On July 15, 2015, 9:38 p.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, lines 110-112 https://reviews.apache.org/r/36518/diff/1/?file=1012851#file1012851line110 Since there's no RESUBSCRIBE, shall we simply call this test 'Subscribe' (I noticed there is no Subscribe test currently) and test the full semantics within it? Looks like a test of the 'Subscribe' call to me. done. On July 15, 2015, 9:38 p.m., Ben Mahler wrote: src/master/master.cpp, line 1747 https://reviews.apache.org/r/36518/diff/1/?file=1012850#file1012850line1747 Seems like we might want to keep the condition consistent across all of these checks (i.e. has_id id != ), up to you. At least, would be nice to add an != operator for FrameworkID vs string. updated the condition. i'll add a TODO to add != operator and do a sweep. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36518/#review91821 --- On July 15, 2015, 6:40 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36518/ --- (Updated July 15, 2015, 6:40 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3055 https://issues.apache.org/jira/browse/MESOS-3055 Repository: mesos Description --- In the process of fixing this, added an additional check in Master::registerFramework() that should've been there in the first place. Similar check exists in Master::reregisterFramework(). Diffs - src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 Diff: https://reviews.apache.org/r/36518/diff/ Testing --- make check Thanks, Vinod Kone
Review Request 36560: Made Subscribe.force optional in the Call protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36560/ --- Review request for mesos and Ben Mahler. Bugs: MESOS-3055 https://issues.apache.org/jira/browse/MESOS-3055 Repository: mesos Description --- Made 'force' optional because it doesn't make sense when subscribing without an id. Diffs - include/mesos/scheduler/scheduler.proto a027da255563c620fa3d7355ad47aa16d2264f77 src/examples/event_call_framework.cpp 17fdcac44c0a51293a318ef5184f4d48a461abd9 src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 Diff: https://reviews.apache.org/r/36560/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36488: Added oversubscription user doc.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36488/#review91988
---
Looks good Nik!
docs/oversubscription.md (line 6)
https://reviews.apache.org/r/36488/#comment145772
for majority of the time
docs/oversubscription.md (line 8)
https://reviews.apache.org/r/36488/#comment145773
we can oversubscribe unallocated resources as well. So probably say
```
Oversubscription takes advantage of temporarily unused resources to ...
```
docs/oversubscription.md (line 28)
https://reviews.apache.org/r/36488/#comment145765
s/resource slack/resource usage slack/
and also allocation slack?
docs/oversubscription.md (lines 30 - 31)
https://reviews.apache.org/r/36488/#comment145767
I thought we use a pull model eventually. So maybe:
```
The slave keeps polling estimates from the resource estimator and tracks
the latest estimate.
```
docs/oversubscription.md (lines 33 - 34)
https://reviews.apache.org/r/36488/#comment145764
The slave only send the message to the master if there's a change to the
previous estimate.
docs/oversubscription.md (line 39)
https://reviews.apache.org/r/36488/#comment145768
The resource is revocable (not the offer).
```
and marks those resources as `revocable`.
```
docs/oversubscription.md (line 40)
https://reviews.apache.org/r/36488/#comment145769
up to the resource estimator to determine which types...
docs/oversubscription.md (line 46)
https://reviews.apache.org/r/36488/#comment145770
Again, it's the resource that's revocable, not the offer.
docs/oversubscription.md (line 49)
https://reviews.apache.org/r/36488/#comment145771
revocable resources.
docs/oversubscription.md (lines 55 - 58)
https://reviews.apache.org/r/36488/#comment145774
I think it's worthwhile to mention that if any resource used by a
task/executor is revocable, the whole container is treated as a revocable
container (meaning can be killed or throttled).
docs/oversubscription.md (line 69)
https://reviews.apache.org/r/36488/#comment145776
Frameworks planning to use oversubscribed resources need to register ...
docs/oversubscription.md (line 80)
https://reviews.apache.org/r/36488/#comment145777
I would say:
```
, the framework will start to receive revocable resources in offers.
```
docs/oversubscription.md (line 84)
https://reviews.apache.org/r/36488/#comment145778
See below for instructions how to configure Mesos for oversubscription.
docs/oversubscription.md (line 86)
https://reviews.apache.org/r/36488/#comment145779
Launching tasks using revocable resources.
docs/oversubscription.md (line 88)
https://reviews.apache.org/r/36488/#comment145780
This is not true currently. Revocable resources and regular resources will
be sent in the same offer.
docs/oversubscription.md (line 95)
https://reviews.apache.org/r/36488/#comment145781
Again, currently, we don't split offers. So you may want to use an example
that has one offer but mixed type of resources.
docs/oversubscription.md (line 148)
https://reviews.apache.org/r/36488/#comment145784
with fixed amount of resources.
docs/oversubscription.md (line 288)
https://reviews.apache.org/r/36488/#comment145792
Maybe you want to also document how to configure the fixed resource
estimator? It's configured through modules, but shipped with Mesos?
```
--resource_estimator=org_apache_mesos_FixedResourceEstimator
--modules=libraries {
file: /usr/local/lib64/libfixed_resource_estimator.so
modules {
name: org_apache_mesos_FixedResourceEstimator
parameters {
key: resources
value: cpus:14
}
}
}
--
```
- Jie Yu
On July 16, 2015, 10:39 p.m., Niklas Nielsen wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36488/
---
(Updated July 16, 2015, 10:39 p.m.)
Review request for mesos, Adam B and Jie Yu.
Repository: mesos
Description
---
Added first draft of oversubscription user doc
Diffs
-
docs/images/oversubscription-overview.jpg PRE-CREATION
docs/oversubscription.md PRE-CREATION
Diff: https://reviews.apache.org/r/36488/diff/
Testing
---
Rendered at:
https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md
Thanks,
Niklas Nielsen
Re: Review Request 36496: Implemented the FAILURE Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36496/#review91933 --- Ship it! Ship It! - Vinod Kone On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36496/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See above. Diffs - src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36496/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review91871 --- Patch looks great! Reviews applied: [36501] All tests passed. - Mesos ReviewBot On July 16, 2015, 5:47 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 16, 2015, 5:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 src/tests/utils.hpp f2eed2e Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36389: Enable remote execution of arbitrary command.
On July 15, 2015, 12:36 a.m., Artem Harutyunyan wrote:
src/slave/slave.cpp, line 4755
https://reviews.apache.org/r/36389/diff/4/?file=1011886#file1011886line4755
Shouldn't there be an equivalent of an assert here if we never expect
this to happen? Something like this:
if (response.isReady()) {
ASSERT
}
return http::BadRequest ...
Unless, there is possibility of a race where the result becomes ready
right at the 15th second (in a separate thread) and by the time this lambda
is executed the response becomes actually (and legitimately) ready.
I sat down with Joris to verify whether or not there was a possible race
condition resulting in a ready Future inside the `.after` lambda, and turned
out that there is not. It's a great idea to have an explicit check for
isReady() to be false, but the assert (i.e. CHECK) still does looks like a
better option.
- Artem
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review91723
---
On July 14, 2015, 4:20 p.m., Marco Massenzio wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/
---
(Updated July 14, 2015, 4:20 p.m.)
Review request for mesos, Benjamin Hindman and Cody Maloney.
Bugs: MESOS-2830
https://issues.apache.org/jira/browse/MESOS-2830
Repository: mesos
Description
---
Jira: MESOS-2830
Under certain (maintenance) circumnstance, it may be necessary
(or desirable) to execute arbitrary operator's commands on the
slave (the entire fleet or a subset thereof) bypassing the Mesos
Task execution mechanism; this may typically be necessary for
maintenance and/or emergency actions.
This patch adds an HTTP endpoint (/execute) which accepts a
JSON-encoded CommandInfo structure and executes the given
command (with optional arguments).
The output of the command (along with potentially any stderr
messages) is returned JSON-encoded in the Response.
For more details, see the design doc at:
https://goo.gl/4npTMU
Diffs
-
src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
Diff: https://reviews.apache.org/r/36389/diff/
Testing
---
make check
lots of manual testing (using Postman, REST client)
Thanks,
Marco Massenzio
Re: Review Request 36501: MESOS-3023
On July 15, 2015, 4:16 a.m., haosdent huang wrote: src/tests/utils.hpp, line 55 https://reviews.apache.org/r/36501/diff/1/?file=1012134#file1012134line55 The code styple need change to follow https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md I've read this code style guidance and updated code accordingly :). Thanks very much for your comments. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review91706 --- On July 16, 2015, 5:47 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 16, 2015, 5:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 src/tests/utils.hpp f2eed2e Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
On July 15, 2015, 4:16 a.m., haosdent huang wrote: src/tests/utils.hpp, line 54 https://reviews.apache.org/r/36501/diff/1/?file=1012134#file1012134line54 I suggest move the implement to cpp file. As far as I known, C++ can not declare template in header file and implete it in cpp. I also have a try with the example. In C++11, it only introduced extern template to avoid duplicated template instance. - Klaus --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review91706 --- On July 16, 2015, 5:47 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 16, 2015, 5:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 src/tests/utils.hpp f2eed2e Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.
On July 9, 2015, 9:19 p.m., Ben Mahler wrote:
3rdparty/libprocess/src/process.cpp, lines 2815-2819
https://reviews.apache.org/r/30032/diff/14/?file=986489#file986489line2815
Any reason we didn't convert os::stat::mtime to return a Time?
The only other user of os::stat::mtime does the same messy conversion:
```
FutureNothing Slave::garbageCollect(const string path)
{
Trylong mtime = os::stat::mtime(path);
if (mtime.isError()) {
LOG(ERROR) Failed to find the mtime of ' path
': mtime.error();
return Failure(mtime.error());
}
// It is unsafe for testing to use unix time directly, we must use
// Time::create to convert into a Time object that reflects the
// possibly advanced state of the libprocess Clock.
TryTime time = Time::create(mtime.get());
CHECK_SOME(time);
```
Till Toenshoff wrote:
One reason could be that os::stat::mtime lives in stout whereas Time
lives in libprocess, no?
Till is right, it is the only reason. `os::stat::mtime` is part of stout while
`Time` is part of libprocess. We look into moving `Time` to stout, but there is
a dependency to the clock which forbids the transition.
On July 9, 2015, 9:19 p.m., Ben Mahler wrote:
3rdparty/libprocess/src/process.cpp, line 2833
https://reviews.apache.org/r/30032/diff/14/?file=986489#file986489line2833
Why did you need the {} here?
It initializes a struct to be zero. It is equivalent to do
`memset(clientMTime, 0, sizeof(clientMTime))`, although my original patch
called for using the more standard `= {0}` some previous reviewer suggested to
change it to the version in the patch.
On July 9, 2015, 9:19 p.m., Ben Mahler wrote:
3rdparty/libprocess/src/process.cpp, lines 2839-2846
https://reviews.apache.org/r/30032/diff/14/?file=986489#file986489line2839
Any reason to prefer -1 special cases here to just using TryTime and
handling errors?
-1 is the value returned in error by `std::mktime`. As mentioned before, we
couldn't use `Time`. Though it is possible to create a `Time` from a `time_t`
initialized with `mktime`. I feel it just adds more points of failure.
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30032/#review91149
---
On June 17, 2015, 5:42 p.m., Alexander Rojas wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30032/
---
(Updated June 17, 2015, 5:42 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad,
Michael Park, and Till Toenshoff.
Bugs: mesos-708
https://issues.apache.org/jira/browse/mesos-708
Repository: mesos
Description
---
When serving a static file, libprocess returns the header `Last-Modified`
which is used by browsers to control Cache.
When a http request arrives containing the header `If-Modified-Since`, a
response `304 Not Modified` is returned if the date in the request and the
modification time (as returned by doing `stat` in the file) coincide.
Unit tests added.
Diffs
-
3rdparty/libprocess/include/process/http.hpp
e47cc7afbc8110759edf25a2dc05d09eda25c417
3rdparty/libprocess/src/process.cpp
a67a3afdb30d23eb1b265b04ae662f64e874b6c6
3rdparty/libprocess/src/tests/process_tests.cpp
660af45e7fd45bdf5d43ad9aa54477fd94f87058
Diff: https://reviews.apache.org/r/30032/diff/
Testing
---
make check
Thanks,
Alexander Rojas
Re: Review Request 35702: Added /reserve HTTP endpoint to the master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35702/#review91889 --- src/master/http.cpp (line 447) https://reviews.apache.org/r/35702/#comment145598 Not directly related to endpoints, but to dynamic reservations in general. Do you think it makes sense to bookkeep dynamic reservation or have an aggregating method in `mesos::internal::master::Role`? - Alexander Rukletsov On June 28, 2015, 8:36 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35702/ --- (Updated June 28, 2015, 8:36 a.m.) Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris Van Remoortere, and Vinod Kone. Bugs: MESOS-2600 https://issues.apache.org/jira/browse/MESOS-2600 Repository: mesos Description --- This involved a lot more challenges than I anticipated, I've captured the various approaches and limitations and deal-breakers of those approaches here: [Master Endpoint Implementation Challenges](https://docs.google.com/document/d/1cwVz4aKiCYP9Y4MOwHYZkyaiuEv7fArCye-vPvB2lAI/edit#) Key points: * This is a stop-gap solution until we shift the offer creation/management logic from the master to the allocator. * `updateAvailable` and `updateSlave` are kept separate because (1) `updateAvailable` is allowed to fail whereas `updateSlave` must not. (2) `updateAvailable` returns a `Future` whereas `updateSlave` does not. (3) `updateAvailable` never leaves the allocator in an over-allocated state and must not, whereas `updateSlave` does, and can. * The algorithm: * Initially, the master pessimistically assume that what seems like available resources will be gone. This is due to the race between the allocator scheduling an `allocate` call to itself vs master's `allocator-updateAvailable` invocation. As such, we first try to satisfy the request only with the offered resources. * We greedily rescind one offer at a time until we've rescinded sufficiently many offers. IMPORTANT: We perform `recoverResources(..., Filters())` rather than `recoverResources(..., None())` so that we can pretty much always win the race against `allocate`. In the case that we lose, no disaster occurs. We simply fail to satisfy the request. * If we still don't have enough resources after resciding all offers, be optimistic and forward the request to the allocator since there may be available resources to satisfy the request. * If the allocator returns a failure, report the error to the user with `PreconditionFailed`. This could be updated to be `Forbidden`, or `Conflict` maybe as well. We'll pick one eventually. This approach is clearly not ideal, since we would prefer to rescind as little offers as possible. The challenges of implementing the ideal solution in the current state is described in the document above. TODO(mpark): Add more comments and test cases. Diffs - src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332 src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064 src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df src/master/validation.hpp 469d6f56c3de28a34177124aae81ce24cb4ad160 src/master/validation.cpp 9d128aa1b349b018b8e4a1916434d848761ca051 Diff: https://reviews.apache.org/r/35702/diff/ Testing --- `make check` Thanks, Michael Park
Re: Review Request 36537: Made TaskState.data available via state.json endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36537/ --- (Updated July 16, 2015, 10:54 a.m.) Review request for mesos, Benjamin Hindman and Timothy Chen. Bugs: MESOS-3061 https://issues.apache.org/jira/browse/MESOS-3061 Repository: mesos Description --- This would allows us to expose the docker container IP (that is queried via docker-inspect and is part of TaskState.data) to Mesos-DNS via Master state.json endpoint. Currently, Master doesn't store TaskState::data and so it's not made available via state.json. A follow up patch would fix it. Diffs - src/common/http.cpp 2bb1ba87a2755a4bd9b762280dea6fce81db1320 Diff: https://reviews.apache.org/r/36537/diff/ Testing --- Tested by modifying the test_executor to send data with TASK_RUNNING status update. The data showed up in Slave's state.json. Thanks, Kapil Arya
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
On July 16, 2015, 2:37 p.m., Joerg Schad wrote: src/launcher/fetcher.cpp, line 128 https://reviews.apache.org/r/36547/diff/1/?file=1013402#file1013402line128 indentation +2 spaces (https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals) see line 61 int he same file for an example :-) - Joerg --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review91885 --- On July 16, 2015, 2:25 p.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 16, 2015, 2:25 p.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 16, 2015, 2:48 p.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs (updated) - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review91885 --- src/launcher/fetcher.cpp (line 123) https://reviews.apache.org/r/36547/#comment145590 s\response\status (to be consistent with below) src/launcher/fetcher.cpp (line 125) https://reviews.apache.org/r/36547/#comment145589 s\successCode\successStatusCode src/launcher/fetcher.cpp (line 128) https://reviews.apache.org/r/36547/#comment145588 indentation +2 spaces (https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals) - Joerg Schad On July 16, 2015, 2:25 p.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 16, 2015, 2:25 p.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review91891 --- src/launcher/fetcher.cpp (line 135) https://reviews.apache.org/r/36547/#comment145601 Suggestion: put the condition check for the protocol AFTER checking if we have an error. Then we do not need the comment above nor the extra variable AND we can state in the error message which specific protocol is meant instead of HTTP/FTP. - Bernd Mathiske On July 16, 2015, 7:48 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 16, 2015, 7:48 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 35983: Added /unreserve HTTP endpoint to the master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35983/#review91890 --- src/master/http.cpp (line 1291) https://reviews.apache.org/r/35983/#comment145599 As in https://reviews.apache.org/r/35702/, I suggest we extract validation into a separate function. src/master/http.cpp (lines 1325 - 1332) https://reviews.apache.org/r/35983/#comment145600 Why do we need to recover resources for unreserve? - Alexander Rukletsov On June 28, 2015, 8:37 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35983/ --- (Updated June 28, 2015, 8:37 a.m.) Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris Van Remoortere, and Vinod Kone. Bugs: MESOS-2600 https://issues.apache.org/jira/browse/MESOS-2600 Repository: mesos Description --- See summary. Diffs - src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332 src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064 src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df Diff: https://reviews.apache.org/r/35983/diff/ Testing --- `make check` Thanks, Michael Park
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review91892 --- Patch looks great! Reviews applied: [36547] All tests passed. - Mesos ReviewBot On July 16, 2015, 2:48 p.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 16, 2015, 2:48 p.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review91904 --- Ship it! Ship It! - Bernd Mathiske On July 16, 2015, 9:55 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 16, 2015, 9:55 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 35715: Added revocable resource state validation.
On June 21, 2015, 11:47 a.m., Vinod Kone wrote:
src/common/resources.cpp, lines 479-487
https://reviews.apache.org/r/35715/diff/1/?file=989223#file989223line479
These checks are done in master's validation.cpp
Michael Park wrote:
Ah sorry, I missed that.
This reminded me of the discussion Jie and I had for
[r32140](https://reviews.apache.org/r/32140/) regarding where validations
should live. I think this validation belongs here rather than in master
validation.
What we concluded from the discussion was that `Resources::validate`
should perform necessary validation to satisfy the invariant of the
`Resource` object.
This enables methods that operate on `Resource` (e.g.
`Resources::isRevocable`) to assume its validity.
My notes:
Synced with Jie on IRC regarding this topic. We agreed that
`Resources::validate` needs to capture the invariant of the `Resource` object
which means it needs to invalidate the `role == * has_reservation()`
state. This invariant is required for all the predicates as well as functions
such as `reserved()` and `unreserved()` to have well-defined behavior.
Jie's note:
Discussed with Mpark offline. We agreed that rule for
Resources::validate is that it should only perform necessary validation to
make sure all methods in Resources are well hahaved, and the validation
around * and reservation info is necessary for 'reserved/unreserved' to work
properly. Thus dropping the issue around validation.
Michael Park wrote:
I found Jie's comment regarding this:
https://reviews.apache.org/r/33865/#comment133597
@Jie: My thought here was that these checks are necessary to make
`isRevocable` well-defined. The same way the check for `* resource cannot
be dynamically reserved` is necessary to make `isDynamicallyReserved` and
others well-defined?
Jie Yu wrote:
@Mpark,
I think the following check is in Resources::validate because otherwise
isReserved will break (e.g., role = `*` and reservation is not set,
isReserved(resource, `*`) will return true).
```
if (resource.role() == * resource.has_reservation()) {
return Error(
Invalid reservation: role \*\ cannot be dynamically reserved);
}
```
Michael Park wrote:
@Jie,
e.g., role = * and reservation is not set, isReserved(resource, *) will
return true
If you meant `role = * and reservation _is_ set`, then yes.
I'm saying that exact reasoning is also why these checks should be in
`Resources::validate`, because otherwise `isRevocable` will break.
e.g. `reservation is set and revocable is set`, `isRevocable` will return
true.
Niklas Nielsen wrote:
Hey guys - did you reach a conclusion?
MPark; how can we get closure on this?
- Niklas
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35715/#review88710
---
On June 21, 2015, noon, Michael Park wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35715/
---
(Updated June 21, 2015, noon)
Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
Repository: mesos
Description
---
In `mesos.proto`, it specifies the expected state of revocable resource:
```
// ... Note that if this is set, 'disk' or 'reservation' cannot be set.
optional RevocableInfo revocable = 9;
```
This expectation should be validated in `Resources::validate(const Resource
resoure)`
Diffs
-
src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
src/tests/resources_tests.cpp 9f96b14a6a4ce416d044934dd7ab4d28e4bc7332
Diff: https://reviews.apache.org/r/35715/diff/
Testing
---
Added `RevocableResourceTest.Validation` + `make check`
Thanks,
Michael Park
Re: Review Request 35715: Added revocable resource state validation.
On June 21, 2015, 6:47 p.m., Vinod Kone wrote:
src/common/resources.cpp, lines 479-487
https://reviews.apache.org/r/35715/diff/1/?file=989223#file989223line479
These checks are done in master's validation.cpp
Michael Park wrote:
Ah sorry, I missed that.
This reminded me of the discussion Jie and I had for
[r32140](https://reviews.apache.org/r/32140/) regarding where validations
should live. I think this validation belongs here rather than in master
validation.
What we concluded from the discussion was that `Resources::validate`
should perform necessary validation to satisfy the invariant of the
`Resource` object.
This enables methods that operate on `Resource` (e.g.
`Resources::isRevocable`) to assume its validity.
My notes:
Synced with Jie on IRC regarding this topic. We agreed that
`Resources::validate` needs to capture the invariant of the `Resource` object
which means it needs to invalidate the `role == * has_reservation()`
state. This invariant is required for all the predicates as well as functions
such as `reserved()` and `unreserved()` to have well-defined behavior.
Jie's note:
Discussed with Mpark offline. We agreed that rule for
Resources::validate is that it should only perform necessary validation to
make sure all methods in Resources are well hahaved, and the validation
around * and reservation info is necessary for 'reserved/unreserved' to work
properly. Thus dropping the issue around validation.
Michael Park wrote:
I found Jie's comment regarding this:
https://reviews.apache.org/r/33865/#comment133597
@Jie: My thought here was that these checks are necessary to make
`isRevocable` well-defined. The same way the check for `* resource cannot
be dynamically reserved` is necessary to make `isDynamicallyReserved` and
others well-defined?
Jie Yu wrote:
@Mpark,
I think the following check is in Resources::validate because otherwise
isReserved will break (e.g., role = `*` and reservation is not set,
isReserved(resource, `*`) will return true).
```
if (resource.role() == * resource.has_reservation()) {
return Error(
Invalid reservation: role \*\ cannot be dynamically reserved);
}
```
Michael Park wrote:
@Jie,
e.g., role = * and reservation is not set, isReserved(resource, *) will
return true
If you meant `role = * and reservation _is_ set`, then yes.
I'm saying that exact reasoning is also why these checks should be in
`Resources::validate`, because otherwise `isRevocable` will break.
e.g. `reservation is set and revocable is set`, `isRevocable` will return
true.
Niklas Nielsen wrote:
Hey guys - did you reach a conclusion?
Niklas Nielsen wrote:
MPark; how can we get closure on this?
I asked Jie to look at it a while ago but I think he's quite busy. I'll discard
it for now.
- Michael
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35715/#review88710
---
On June 21, 2015, 7 p.m., Michael Park wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35715/
---
(Updated June 21, 2015, 7 p.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
Repository: mesos
Description
---
In `mesos.proto`, it specifies the expected state of revocable resource:
```
// ... Note that if this is set, 'disk' or 'reservation' cannot be set.
optional RevocableInfo revocable = 9;
```
This expectation should be validated in `Resources::validate(const Resource
resoure)`
Diffs
-
src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521
src/tests/resources_tests.cpp 9f96b14a6a4ce416d044934dd7ab4d28e4bc7332
Diff: https://reviews.apache.org/r/35715/diff/
Testing
---
Added `RevocableResourceTest.Validation` + `make check`
Thanks,
Michael Park
Re: Review Request 34361: converted hard-coded strings to consts
On June 9, 2015, 6:25 p.m., Ben Mahler wrote: src/tests/master_tests.cpp, lines 3031-3034 https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031 Why bother with all this? Why not just have `key1`, `value1`, `key2`, `value2` inlined appropriately throughout these tests. Very straightforward to read. Colin Williams wrote: I think the issue with the changes remaining is that the test depends on the same value occurring in several places. By consolidating to a variable it's no longer possible for these values to get out of sync. Niklas Nielsen wrote: Colin: exactly Ben: We have label tests three places; in the master, slave and in the modules and they use the same foo, bar, baz, qux key value pairs. The idea was to centralize them, so they don't get out of date and to avoid code duplication. Does that make sense? Ben Mahler wrote: Well, then let's just keep it simple and get rid of these special strings by just making the tests use key1, value1, key2, value2, etc. I'm not following your code duplication point, this introduces more code (now there are additional global constants, which we like to avoid if unnecessary). Also, each test is ideally independent, why does the master's test for labels affect the slave's test for labels? Let's say I need a test with 5 labels, you want to have 5x2=10 global constants to address this? Niklas Nielsen wrote: Try to see how testLabelKey and testLabelValue was used previously and foo, bar, qux was used on others and I created this ticket to unify the way we do this, along with seeing these key value pair being created in the slave and master tests. Dispite the current patch, I do think we can simply and unify the test label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious which key pairs are being tested. The comments in the test code need to carry a bunch of context, to make sense out of the label transformations (especially across the library border for the hooks tests). This is all in spirit of reducing the tech debt we introduced. We are on the same page on not introducing more lines/key pairs than before. Let us iterate on this and get back to you. Colin Williams wrote: Ben: I'm more in agreement with your sentiment, I'm not sure I see the point of unifying label names into a centralized variable that aren't at all related. Niklas: I was a little confused by the original task description, so I thought a sample patch would be a good discussion point. I don't entirely understand the tech debt you're trying to solve. For example, it seems like you're asking to have some constants defined somewhere that are used by both master_test and slave_test, but the the similarities between these two only seem incidental. I'm concerned that creating something like CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code. Niklas Nielsen wrote: Okay - thinking about this; I am on board with key1, value1 etc. Colin: the tech debt is that we have some inlined constants (like foo, bar, etc) and some which are constants (which have to be kept in sync between hooks modules and test body). The idea was to unify the way we name the key value pairs. Do you want to update this ticket to reflect this, or come with a new patch set which tackles streaming the two? Niklas Nielsen wrote: Ping :) Colin Williams wrote: Sorry, my day job's been really all-consuming the last few days. I was planning to update the ticket. Colin Williams wrote: Ticket updated Colin; thanks! should we keep this patch then, or will you be providing a new one? - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34361/#review87331 --- On June 8, 2015, 12:05 p.m., Colin Williams wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34361/ --- (Updated June 8, 2015, 12:05 p.m.) Review request for mesos and Niklas Nielsen. Bugs: MESOS-2637 https://issues.apache.org/jira/browse/MESOS-2637 Repository: mesos Description --- converted hard-coded strings to consts Diffs - src/tests/hook_tests.cpp 3ffde6d src/tests/master_tests.cpp ba3858f src/tests/slave_tests.cpp acae497 Diff: https://reviews.apache.org/r/34361/diff/ Testing --- Thanks, Colin Williams
Re: Review Request 36537: Made TaskState.data available via state.json endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36537/#review91895 --- Patch looks great! Reviews applied: [36537] All tests passed. - Mesos ReviewBot On July 16, 2015, 2:54 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36537/ --- (Updated July 16, 2015, 2:54 p.m.) Review request for mesos, Benjamin Hindman and Timothy Chen. Bugs: MESOS-3061 https://issues.apache.org/jira/browse/MESOS-3061 Repository: mesos Description --- This would allows us to expose the docker container IP (that is queried via docker-inspect and is part of TaskState.data) to Mesos-DNS via Master state.json endpoint. Currently, Master doesn't store TaskState::data and so it's not made available via state.json. A follow up patch would fix it. Diffs - src/common/http.cpp 2bb1ba87a2755a4bd9b762280dea6fce81db1320 Diff: https://reviews.apache.org/r/36537/diff/ Testing --- Tested by modifying the test_executor to send data with TASK_RUNNING status update. The data showed up in Slave's state.json. Thanks, Kapil Arya
Re: Review Request 36494: Implemented the MESSAGE Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36494/#review91928 --- Ship it! src/tests/scheduler_event_call_tests.cpp (line 91) https://reviews.apache.org/r/36494/#comment145658 Can you add a comment here saying that you are simulating master sending the event? - Vinod Kone On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36494/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See above. Diffs - src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36494/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 35797: Updated Frameworkinfo.capabilities on framework re-registration to support adding capabilities
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35797/#review91948 --- Patch looks great! Reviews applied: [35797] All tests passed. - Mesos ReviewBot On July 16, 2015, 6:46 p.m., Aditi Dixit wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35797/ --- (Updated July 16, 2015, 6:46 p.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2880 https://issues.apache.org/jira/browse/MESOS-2880 Repository: mesos Description --- This only updates the master, not the allocator. Added test too. Diffs - src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e Diff: https://reviews.apache.org/r/35797/diff/ Testing --- make check Thanks, Aditi Dixit
Re: Review Request 36494: Implemented the MESSAGE Event handler in the scheduler driver.
On July 16, 2015, 6:20 p.m., Vinod Kone wrote: src/tests/scheduler_event_call_tests.cpp, line 91 https://reviews.apache.org/r/36494/diff/1/?file=1011987#file1011987line91 Can you add a comment here saying that you are simulating master sending the event? Hm.. I'll have to scatter this comment across all the tests I've added in this chain, I'll add a comment at the 'SchedulerDriverEventTest' level instead since all the tests post events manually. Good? - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36494/#review91928 --- On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36494/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See above. Diffs - src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36494/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 36495: Implemented the RESCIND Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36495/#review91929 --- Ship it! src/sched/sched.cpp (line 469) https://reviews.apache.org/r/36495/#comment145661 Add a TODO here to refactor. src/tests/scheduler_event_call_tests.cpp (line 85) https://reviews.apache.org/r/36495/#comment145659 ditto. comment. - Vinod Kone On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36495/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See above. Diffs - src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36495/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 36537: Made TaskState.data available via state.json endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36537/#review91937 --- We don't store 'data' because there are frameworks which send a lot of data, and this can OOM the master per MESOS-1746. Are you aware of this? - Ben Mahler On July 16, 2015, 2:54 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36537/ --- (Updated July 16, 2015, 2:54 p.m.) Review request for mesos, Benjamin Hindman and Timothy Chen. Bugs: MESOS-3061 https://issues.apache.org/jira/browse/MESOS-3061 Repository: mesos Description --- This would allows us to expose the docker container IP (that is queried via docker-inspect and is part of TaskState.data) to Mesos-DNS via Master state.json endpoint. Currently, Master doesn't store TaskState::data and so it's not made available via state.json. A follow up patch would fix it. Diffs - src/common/http.cpp 2bb1ba87a2755a4bd9b762280dea6fce81db1320 Diff: https://reviews.apache.org/r/36537/diff/ Testing --- Tested by modifying the test_executor to send data with TASK_RUNNING status update. The data showed up in Slave's state.json. Thanks, Kapil Arya
Re: Review Request 35797: Updated Frameworkinfo.capabilities on framework re-registration to support adding capabilities
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35797/ --- (Updated July 16, 2015, 6:46 p.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2880 https://issues.apache.org/jira/browse/MESOS-2880 Repository: mesos Description (updated) --- This only updates the master, not the allocator. Added test too. Diffs (updated) - src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e Diff: https://reviews.apache.org/r/35797/diff/ Testing --- make check Thanks, Aditi Dixit
Re: Review Request 36493: Added a stub Event message handler in the scheduler driver.
On July 15, 2015, 7 p.m., Vinod Kone wrote: src/sched/sched.cpp, line 437 https://reviews.apache.org/r/36493/diff/1/?file=1011982#file1011982line437 Do you know why it doesn't resolve? Does stringify() help? It should from what I can tell since we are within mesos::internal and the operators are in mesos::, but it turns out that gcc at least isn't resolving them and is just printing 1, 2, etc. I noticed that this is also the case for our other example frameworks (e.g. no_executor_framework). - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36493/#review91794 --- On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36493/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See MESOS-2910. Diffs - src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f Diff: https://reviews.apache.org/r/36493/diff/ Testing --- Each event is tested in the follow up patches. Thanks, Ben Mahler
Re: Review Request 36498: Implemented the OFFERS Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36498/#review91939 --- Ship it! src/sched/sched.cpp (lines 494 - 495) https://reviews.apache.org/r/36498/#comment145685 couldn't you have used ANY which is the default? src/tests/scheduler_event_call_tests.cpp (line 176) https://reviews.apache.org/r/36498/#comment145691 This test also ensures that framework to executor messages can bypass the master right? Mind adding that here? src/tests/scheduler_event_call_tests.cpp (line 211) https://reviews.apache.org/r/36498/#comment145693 s/offers/ResourceOfferMessages/ src/tests/scheduler_event_call_tests.cpp (lines 230 - 235) https://reviews.apache.org/r/36498/#comment145694 use createTask()? src/tests/scheduler_event_call_tests.cpp (line 237) https://reviews.apache.org/r/36498/#comment145695 Why capture the executor driver when it's not used? - Vinod Kone On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36498/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 and MESOS-3012 https://issues.apache.org/jira/browse/MESOS-2910 https://issues.apache.org/jira/browse/MESOS-3012 Repository: mesos Description --- This relies on 'Offer.url' to compute the pids needed for the message passing optimization (see [MESOS-3012](https://issues.apache.org/jira/browse/MESOS-3012)). Diffs - src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36498/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 36450: Introduced Address and URL protobufs.
On July 15, 2015, 7:08 p.m., Vinod Kone wrote: src/common/type_utils.cpp, line 131 https://reviews.apache.org/r/36450/diff/2/?file=1011909#file1011909line131 Is the order of query parameters important? Aren't these URLs equivalent? http://a.b.c/?k1=ak2=b http://a.b.c/?k2=bk1=a Java's URI class considers ordering as important: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equals%28java.lang.Object%29 I'd also like to keep it simple for now, you'll notice that they consider percent encoding to be case-insensitive (e.g. %2C == %2c), but I'd hope we can just avoid this for now: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equal%28java.lang.String%2Cjava.lang.String%29 Ideally, we'd have a URI / URL class in stout, where we can have a comprehensive equality operator. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/#review91795 --- On July 15, 2015, 1:01 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/ --- (Updated July 15, 2015, 1:01 a.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone. Bugs: MESOS-3012 https://issues.apache.org/jira/browse/MESOS-3012 Repository: mesos Description --- To make the API more consistent, we'd like to have a single way to express a network address. Also would like a way to express an HTTP address (a URL), which may include a path prefix. This also enables the message passing optimization in the scheduler driver when receiving events, per MESOS-3012. Diffs - include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 Diff: https://reviews.apache.org/r/36450/diff/ Testing --- Modified the simplest test I could find for offers. Thanks, Ben Mahler
Re: Review Request 36450: Introduced Address and URL protobufs.
On July 15, 2015, 7:08 p.m., Vinod Kone wrote: src/common/type_utils.cpp, line 131 https://reviews.apache.org/r/36450/diff/2/?file=1011909#file1011909line131 Is the order of query parameters important? Aren't these URLs equivalent? http://a.b.c/?k1=ak2=b http://a.b.c/?k2=bk1=a Ben Mahler wrote: Java's URI class considers ordering as important: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equals%28java.lang.Object%29 I'd also like to keep it simple for now, you'll notice that they consider percent encoding to be case-insensitive (e.g. %2C == %2c), but I'd hope we can just avoid this for now: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equal%28java.lang.String%2Cjava.lang.String%29 Ideally, we'd have a URI / URL class in stout, where we can have a comprehensive equality operator. +1 There is already a URL struct that exists in libprocess that we might consider enriching upon later and also move it to stout. - Anand --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/#review91795 --- On July 15, 2015, 1:01 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/ --- (Updated July 15, 2015, 1:01 a.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone. Bugs: MESOS-3012 https://issues.apache.org/jira/browse/MESOS-3012 Repository: mesos Description --- To make the API more consistent, we'd like to have a single way to express a network address. Also would like a way to express an HTTP address (a URL), which may include a path prefix. This also enables the message passing optimization in the scheduler driver when receiving events, per MESOS-3012. Diffs - include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 Diff: https://reviews.apache.org/r/36450/diff/ Testing --- Modified the simplest test I could find for offers. Thanks, Ben Mahler
Re: Review Request 36537: Made TaskState.data available via state.json endpoint.
On July 16, 2015, 6:38 p.m., Ben Mahler wrote: We don't store 'data' because there are frameworks which send a lot of data, and this can OOM the master per MESOS-1746. Are you aware of this? Kapil Arya wrote: Yes. That's why I haven't created the patch yet :-). I am still trying to explore avenues that can allow us to expose the `docker inspect` output _only_. One somewhat hackish way is to detect and save `docker inspect` run. An alternative is to create TaskStatus labels that are exposed via state.json. The latter would also allow us to create label decorator hooks for Slave modules. Isn't sending docker inspect already a hack? What if folks have a custom executor in their docker container? Then they won't be getting 'docker inspect' from the docker/executor.cpp, right? - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36537/#review91937 --- On July 16, 2015, 2:54 p.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36537/ --- (Updated July 16, 2015, 2:54 p.m.) Review request for mesos, Benjamin Hindman and Timothy Chen. Bugs: MESOS-3061 https://issues.apache.org/jira/browse/MESOS-3061 Repository: mesos Description --- This would allows us to expose the docker container IP (that is queried via docker-inspect and is part of TaskState.data) to Mesos-DNS via Master state.json endpoint. Currently, Master doesn't store TaskState::data and so it's not made available via state.json. A follow up patch would fix it. Diffs - src/common/http.cpp 2bb1ba87a2755a4bd9b762280dea6fce81db1320 Diff: https://reviews.apache.org/r/36537/diff/ Testing --- Tested by modifying the test_executor to send data with TASK_RUNNING status update. The data showed up in Slave's state.json. Thanks, Kapil Arya
Re: Review Request 35797: Updated Frameworkinfo.capabilities on framework re-registration to support adding capabilities
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35797/#review91955
---
src/master/master.hpp (line 534)
https://reviews.apache.org/r/35797/#comment145723
s/capabilities_size()/capabilities.size()/
src/master/master.hpp (line 536)
https://reviews.apache.org/r/35797/#comment145722
Do you also want to deal with the case where capabilities are previously
set but now being unset? In other words, the else case.
if (source.capabilities.size() 0 ) {
info.mutable_capabilities()-CopyFrom(source.capabilities);
} else {
info.clear_capabilities();
}
If you want do this in a follow up patch, add a TODO here. You will also
need a new test for this.
src/tests/fault_tolerance_tests.cpp (line 1730)
https://reviews.apache.org/r/35797/#comment145724
Add a comment on the top of the test that you are verifying that when a
framework re-registers with updated framework info, it gets updated in the
master.
src/tests/fault_tolerance_tests.cpp (lines 1742 - 1743)
https://reviews.apache.org/r/35797/#comment145720
We now require a much newer gcc version, so this shouldn't be an issue. You
can just do.
FrameworkInfo framework1 = DEFAULT_FRAMEWORK_INFO;
src/tests/fault_tolerance_tests.cpp (lines 1769 - 1770)
https://reviews.apache.org/r/35797/#comment145725
ditto. just assign on the same line.
src/tests/fault_tolerance_tests.cpp (line 1772)
https://reviews.apache.org/r/35797/#comment145726
s/Name/Type/
src/tests/fault_tolerance_tests.cpp (lines 1773 - 1774)
https://reviews.apache.org/r/35797/#comment145730
Can you also update webui_url, hostname and failover_timeout while you are
at it?
src/tests/fault_tolerance_tests.cpp (line 1803)
https://reviews.apache.org/r/35797/#comment145732
s/masterState/response/
src/tests/fault_tolerance_tests.cpp (line 1806)
https://reviews.apache.org/r/35797/#comment145733
s/masterStateObject/parse/
you also need a
ASSERT_SOME(parse);
src/tests/fault_tolerance_tests.cpp (line 1809)
https://reviews.apache.org/r/35797/#comment145734
new line after ASSERT.
s/masterStateObject_/state/
src/tests/fault_tolerance_tests.cpp (line 1812)
https://reviews.apache.org/r/35797/#comment145736
s/famework_/framework/
new line after this.
src/tests/fault_tolerance_tests.cpp (line 1831)
https://reviews.apache.org/r/35797/#comment145737
Nice test!
- Vinod Kone
On July 16, 2015, 6:46 p.m., Aditi Dixit wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35797/
---
(Updated July 16, 2015, 6:46 p.m.)
Review request for mesos and Vinod Kone.
Bugs: MESOS-2880
https://issues.apache.org/jira/browse/MESOS-2880
Repository: mesos
Description
---
This only updates the master, not the allocator. Added test too.
Diffs
-
src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec
src/tests/fault_tolerance_tests.cpp
1070ccf17f98f6b3800684a5edd6517d90631c3e
Diff: https://reviews.apache.org/r/35797/diff/
Testing
---
make check
Thanks,
Aditi Dixit
Re: Review Request 36450: Introduced Address and URL protobufs.
On July 15, 2015, 7:08 p.m., Vinod Kone wrote: src/common/type_utils.cpp, line 131 https://reviews.apache.org/r/36450/diff/2/?file=1011909#file1011909line131 Is the order of query parameters important? Aren't these URLs equivalent? http://a.b.c/?k1=ak2=b http://a.b.c/?k2=bk1=a Ben Mahler wrote: Java's URI class considers ordering as important: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equals%28java.lang.Object%29 I'd also like to keep it simple for now, you'll notice that they consider percent encoding to be case-insensitive (e.g. %2C == %2c), but I'd hope we can just avoid this for now: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URI.java#URI.equal%28java.lang.String%2Cjava.lang.String%29 Ideally, we'd have a URI / URL class in stout, where we can have a comprehensive equality operator. Anand Mazumdar wrote: +1 There is already a URL struct that exists in libprocess that we might consider enriching upon later and also move it to stout. I'll add a TODO related to this. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/#review91795 --- On July 15, 2015, 1:01 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/ --- (Updated July 15, 2015, 1:01 a.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone. Bugs: MESOS-3012 https://issues.apache.org/jira/browse/MESOS-3012 Repository: mesos Description --- To make the API more consistent, we'd like to have a single way to express a network address. Also would like a way to express an HTTP address (a URL), which may include a path prefix. This also enables the message passing optimization in the scheduler driver when receiving events, per MESOS-3012. Diffs - include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 Diff: https://reviews.apache.org/r/36450/diff/ Testing --- Modified the simplest test I could find for offers. Thanks, Ben Mahler
Re: Review Request 36494: Implemented the MESSAGE Event handler in the scheduler driver.
On July 16, 2015, 6:20 p.m., Vinod Kone wrote: src/tests/scheduler_event_call_tests.cpp, line 91 https://reviews.apache.org/r/36494/diff/1/?file=1011987#file1011987line91 Can you add a comment here saying that you are simulating master sending the event? Ben Mahler wrote: Hm.. I'll have to scatter this comment across all the tests I've added in this chain, I'll add a comment at the 'SchedulerDriverEventTest' level instead since all the tests post events manually. Good? sg. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36494/#review91928 --- On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36494/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See above. Diffs - src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36494/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 36496: Implemented the FAILURE Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36496/#review91932 --- src/sched/sched.cpp (line 509) https://reviews.apache.org/r/36496/#comment145663 TODO for refactor. src/tests/scheduler_event_call_tests.cpp (line 170) https://reviews.apache.org/r/36496/#comment145664 comment. - Vinod Kone On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36496/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See above. Diffs - src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36496/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 36499: Implemented the UPDATE Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36499/#review91942 --- Ship it! src/sched/sched.cpp (line 547) https://reviews.apache.org/r/36499/#comment145703 TODO refactor. src/tests/scheduler_event_call_tests.cpp (lines 331 - 332) https://reviews.apache.org/r/36499/#comment145704 why this check? src/tests/scheduler_event_call_tests.cpp (line 380) https://reviews.apache.org/r/36499/#comment145705 No need for driver.stop() and driver.join()? Shutdown()? - Vinod Kone On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36499/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See above. Diffs - include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36499/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 36537: Made TaskState.data available via state.json endpoint.
On July 16, 2015, 2:38 p.m., Ben Mahler wrote: We don't store 'data' because there are frameworks which send a lot of data, and this can OOM the master per MESOS-1746. Are you aware of this? Yes. That's why I haven't created the patch yet :-). I am still trying to explore avenues that can allow us to expose the `docker inspect` output _only_. One somewhat hackish way is to detect and save `docker inspect` run. An alternative is to create TaskStatus labels that are exposed via state.json. The latter would also allow us to create label decorator hooks for Slave modules. - Kapil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36537/#review91937 --- On July 16, 2015, 10:54 a.m., Kapil Arya wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36537/ --- (Updated July 16, 2015, 10:54 a.m.) Review request for mesos, Benjamin Hindman and Timothy Chen. Bugs: MESOS-3061 https://issues.apache.org/jira/browse/MESOS-3061 Repository: mesos Description --- This would allows us to expose the docker container IP (that is queried via docker-inspect and is part of TaskState.data) to Mesos-DNS via Master state.json endpoint. Currently, Master doesn't store TaskState::data and so it's not made available via state.json. A follow up patch would fix it. Diffs - src/common/http.cpp 2bb1ba87a2755a4bd9b762280dea6fce81db1320 Diff: https://reviews.apache.org/r/36537/diff/ Testing --- Tested by modifying the test_executor to send data with TASK_RUNNING status update. The data showed up in Slave's state.json. Thanks, Kapil Arya
Re: Review Request 36499: Implemented the UPDATE Event handler in the scheduler driver.
On July 16, 2015, 6:57 p.m., Vinod Kone wrote: src/tests/scheduler_event_call_tests.cpp, lines 331-332 https://reviews.apache.org/r/36499/diff/1/?file=1011999#file1011999line331 why this check? I need the frameworkId from the message, so wanted to make sure the message parsed correctly before grabbing the framework id. On July 16, 2015, 6:57 p.m., Vinod Kone wrote: src/tests/scheduler_event_call_tests.cpp, line 380 https://reviews.apache.org/r/36499/diff/1/?file=1011999#file1011999line380 No need for driver.stop() and driver.join()? Shutdown()? The driver destructor and the mesos test cleanup look sufficient. Was there something about this test that made you ask? Note that I don't do explicit stopping, joining, shutdown in the other tests. - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36499/#review91942 --- On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36499/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See above. Diffs - include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36499/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 36497: Implemented the SUBSCRIBE Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36497/#review91934 --- src/sched/sched.cpp (lines 453 - 454) https://reviews.apache.org/r/36497/#comment145667 Do you want to add a CHECK to make sure that 'master' is also None? I propose that we get rid of 'master' altogether. See my comment below. src/sched/sched.cpp (line 463) https://reviews.apache.org/r/36497/#comment145669 except for the 3rd case. can you add that to the comment. you can copy paste what you wrote in the description of this review. src/sched/sched.cpp (line 1393) https://reviews.apache.org/r/36497/#comment145665 Why store both 'master' and 'masterInfo'? I think you can get rid of 'master'. Gets us away from having to make sure they are in sync. src/tests/scheduler_event_call_tests.cpp (line 59) https://reviews.apache.org/r/36497/#comment145670 Can you expand on the comment here? This test is a bit complicated and could use some comments on what you are doing and testing. src/tests/scheduler_event_call_tests.cpp (line 73) https://reviews.apache.org/r/36497/#comment145674 Needs a comment on why you are dropping the message. src/tests/scheduler_event_call_tests.cpp (line 76) https://reviews.apache.org/r/36497/#comment145681 why pausing the clock? comment. src/tests/scheduler_event_call_tests.cpp (lines 83 - 84) https://reviews.apache.org/r/36497/#comment145671 Why are you doing this test? src/tests/scheduler_event_call_tests.cpp (lines 89 - 91) https://reviews.apache.org/r/36497/#comment145673 pull this below the expectation. src/tests/scheduler_event_call_tests.cpp (line 97) https://reviews.apache.org/r/36497/#comment145672 comment. src/tests/scheduler_event_call_tests.cpp (line 101) https://reviews.apache.org/r/36497/#comment145675 s/call/message/ src/tests/scheduler_event_call_tests.cpp (line 115) https://reviews.apache.org/r/36497/#comment145676 comment. src/tests/scheduler_event_call_tests.cpp (lines 119 - 121) https://reviews.apache.org/r/36497/#comment145677 hmm. can you split this into its own test. this test is huge! src/tests/scheduler_event_call_tests.cpp (line 139) https://reviews.apache.org/r/36497/#comment145679 comment. src/tests/scheduler_event_call_tests.cpp (line 145) https://reviews.apache.org/r/36497/#comment145678 ditto. split this into its own test. src/tests/scheduler_event_call_tests.cpp (line 158) https://reviews.apache.org/r/36497/#comment145680 comment. - Vinod Kone On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36497/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- This one is non-trivial, note that we follow MESOS-786 with the exception of the 3rd case, since it is not possible and schedulers could not have possibly relied on getting registered instead of re-registered in that case. We now need to store the MasterInfo coming from the detector, as it is not provided in Event. Diffs - src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36497/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 36463: Updated scheduler driver to send Kill Call.
On July 15, 2015, 5:58 p.m., Ben Mahler wrote: src/tests/fault_tolerance_tests.cpp, lines 1359-1360 https://reviews.apache.org/r/36463/diff/1/?file=1010279#file1010279line1359 Ditto here, do we need the mesos:: prefix? yea, unfortunately it's needed, because otherwise the compiler gets confused as it looks in the mesos::internal::scheduler namespace - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36463/#review91762 --- On July 13, 2015, 11:58 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36463/ --- (Updated July 13, 2015, 11:58 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 Diff: https://reviews.apache.org/r/36463/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36463: Updated scheduler driver to send Kill Call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36463/ --- (Updated July 17, 2015, 2:26 a.m.) Review request for mesos and Ben Mahler. Changes --- benm's. NNFR. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs (updated) - src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc Diff: https://reviews.apache.org/r/36463/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34128: Enable different IP/Port for external access.
On June 11, 2015, 7:34 p.m., Vinod Kone wrote: 3rdparty/libprocess/src/process.cpp, lines 820-836 https://reviews.apache.org/r/34128/diff/2/?file=963212#file963212line820 If two libprocess based unix processes (e.g., scheudler and master) are within the *same* bridged container, would they able to communicate with this change? Can you test this to confirm? If not, a better option might be to instead have LIBPROCESS_BIND_IP and LIBPROCESS_BIND_PORT that just changes the address we bind to. LIBPROCESS_IP and LIBPROCESS_PORT semantics could be left untouched. Anindya Sinha wrote: If 2 libprocess based unix processes are running, they would point to a different public_ip:public_port (most likely same public_ip but a different public_port, ie. same LIBPROCESS_PUBLIC_IP but a different LIBPROCESS_PUBLIC_PORT). The processes themselves would bind as it does today on ip:port (based in LIBPROCESS_IP and LIBPROCESS_PORT). Once a request lands on a corresponding public_ip:public_port, a proxy listening on that would forward that to the actual ip:port corresponding to the public_ip:public_port. As an example, mesos-master binds on 10.11.12.13:5050 (ip:port) with public_ip:public_port as 192.168.100.100:6050, and say scheduler binds on 10.11.12.13:8081 with public_ip:public_port as 192.168.100.100:9081. Requests received on 192.168.100.100:6050 shall be proxied over to 10.11.12.13:5050 (to reach mesos-master) and requests received on 192.168.100.100:9081 shall be proxied over to 10.11.12.13:8081 (to reach scheduler). Vinod Kone wrote: ``` a proxy listening on that would forward that... ``` who sets up this proxy? or do you mean this is what happens currently in bridged mode containers (e.g., docker)? Anindya Sinha wrote: No this is not something that is part of docker. The proxy would be something external to the container which would proxy the request to your application within the container. This patch enables external entities viz. zookeeper reach the host (based on public_ip:public_port) from where a proxy will be required to route to the application running within the container. If we use ip:port, zookeeper won't be able to reach. The setting up of the proxy is not a part of docker or libprocess. Please also refer to relevant issue https://issues.apache.org/jira/browse/MESOS-2587. Can we get some inputs on this so as to move forward? Please also look into https://reviews.apache.org/r/34129/ which is also a part of this fix. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34128/#review87611 --- On May 18, 2015, 10:08 p.m., Anindya Sinha wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34128/ --- (Updated May 18, 2015, 10:08 p.m.) Review request for mesos. Bugs: MESOS-809 https://issues.apache.org/jira/browse/MESOS-809 Repository: mesos Description --- Expose environment variables LIBPROCESS_PUBLIC_IP and LIBPROCESS_PUBLIC_PORT as the IP and port which libprocess would advertise (if set). If not set, it defaults to the IP and port on which it binded to. Diffs - 3rdparty/libprocess/src/process.cpp e3de3cd6b536aaaf59784360aed546512dd04dc9 Diff: https://reviews.apache.org/r/34128/diff/ Testing --- Testing: make test Thanks, Anindya Sinha
Re: Review Request 36424: Created a command executor helper method.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/ --- (Updated July 17, 2015, 5:52 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, and Cody Maloney. Changes --- Added comments from Ben M. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description (updated) --- Jira: MESOS-2902 While researching how to execute an arbitrary script to detect the Master IP address, it emerged clearly that a helper method to execute an arbitrary command/script on a node and obtain either stdout or stderr would have been useful and avoided a lot of code repetition. This could not be ultimately used for the purpose at hand, but I believe it to be useful enough (particularly, to avoid people doing coding by copypaste and/or waste time researching the same functionality). This would also be beneficial in MESOS-2830 and MESOS-2834 factoring out the remote command execution logic. Diffs - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4 Diff: https://reviews.apache.org/r/36424/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 36464: Updated scheduler driver to send ACCEPT call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36464/ --- (Updated July 17, 2015, 3:14 a.m.) Review request for mesos and Ben Mahler. Changes --- benm's. NNFR. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs (updated) - src/sched/sched.cpp de76803581d32d7f7e93aac1678e3a2eba577e73 src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc Diff: https://reviews.apache.org/r/36464/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36498: Implemented the OFFERS Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36498/ --- (Updated July 17, 2015, 1:36 a.m.) Review request for mesos and Vinod Kone. Changes --- Cleanups per Vinod's review. Bugs: MESOS-2910 and MESOS-3012 https://issues.apache.org/jira/browse/MESOS-2910 https://issues.apache.org/jira/browse/MESOS-3012 Repository: mesos Description --- This relies on 'Offer.url' to compute the pids needed for the message passing optimization (see [MESOS-3012](https://issues.apache.org/jira/browse/MESOS-3012)). Diffs (updated) - src/sched/sched.cpp 839fcbf0169fecf74fb17cab94fe4d35a1e20a10 src/tests/scheduler_event_call_tests.cpp cf6aa19a644580ff9d805e919763e9342d72677f Diff: https://reviews.apache.org/r/36498/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 36450: Introduced Address and URL protobufs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/ --- (Updated July 17, 2015, 1:36 a.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone. Changes --- Added a TODO to use process::URL equality. Bugs: MESOS-3012 https://issues.apache.org/jira/browse/MESOS-3012 Repository: mesos Description --- To make the API more consistent, we'd like to have a single way to express a network address. Also would like a way to express an HTTP address (a URL), which may include a path prefix. This also enables the message passing optimization in the scheduler driver when receiving events, per MESOS-3012. Diffs (updated) - include/mesos/mesos.proto d2f482668e671b30f2586f6aae9c20132ab4d1e4 include/mesos/type_utils.hpp eb7fe2562cfcff52288d1c216425068d1ba551c0 src/common/type_utils.cpp 2ad5b4cbe324c83e81fd7df7430652f5c0a4e30f src/master/master.cpp 082758ef54597ad32cf0d025c147f0f44dd11961 src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc Diff: https://reviews.apache.org/r/36450/diff/ Testing --- Modified the simplest test I could find for offers. Thanks, Ben Mahler
Re: Review Request 36467: Updated scheduler driver to send ACKNOWLEDGE call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36467/ --- (Updated July 17, 2015, 3:16 a.m.) Review request for mesos and Ben Mahler. Changes --- benm's NNFR. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs (updated) - src/sched/sched.cpp de76803581d32d7f7e93aac1678e3a2eba577e73 src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e src/tests/reconciliation_tests.cpp 6042d8c02d86f486e0c4d82d5a70666d7ac9019b src/tests/scheduler_tests.cpp 2ce280a9b153263130694820c101cbad29471179 src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 src/tests/slave_tests.cpp 89cc7f68b33b037626ca6056647c360b5a6d5901 src/tests/status_update_manager_tests.cpp 440b07475e28dc491ab640acaca8ee20db8411f8 Diff: https://reviews.apache.org/r/36467/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36469: Updated scheduler driver to send MESSAGE call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36469/ --- (Updated July 17, 2015, 3:16 a.m.) Review request for mesos and Ben Mahler. Changes --- benm's. NNFR. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs (updated) - src/sched/sched.cpp de76803581d32d7f7e93aac1678e3a2eba577e73 src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 Diff: https://reviews.apache.org/r/36469/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36469: Updated scheduler driver to send MESSAGE call.
On July 15, 2015, 5:58 p.m., Ben Mahler wrote: src/tests/fault_tolerance_tests.cpp, lines 1313-1315 https://reviews.apache.org/r/36469/diff/1/?file=1010296#file1010296line1313 We don't need to worry about gcc 4.1.* anymore, you can assign now on the same line :) copy paste error. fixed. On July 15, 2015, 5:58 p.m., Ben Mahler wrote: src/tests/fault_tolerance_tests.cpp, line 1265 https://reviews.apache.org/r/36469/diff/1/?file=1010296#file1010296line1265 Did you want to expect that the message is sent to through the master using call, since it looks like no offers go to the second scheduler? sure thing. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36469/#review91770 --- On July 14, 2015, 12:30 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36469/ --- (Updated July 14, 2015, 12:30 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e Diff: https://reviews.apache.org/r/36469/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36547: Fixed fetcher failing for FTP URIs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/#review92015 --- src/launcher/fetcher.cpp (lines 127 - 128) https://reviews.apache.org/r/36547/#comment145870 Why not just check for any 2xx code then? Aren't they all successful in one way or another? - Adam B On July 16, 2015, 9:55 a.m., Jan Schlicht wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36547/ --- (Updated July 16, 2015, 9:55 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-3060 https://issues.apache.org/jira/browse/MESOS-3060 Repository: mesos Description --- The response code for successful FTP file transfers is 226, while it is 200 for HTTP. The fetcher has been changed to check for a response code of 226 for FTP URIs. Diffs - src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd Diff: https://reviews.apache.org/r/36547/diff/ Testing --- make check external FTP server test. Thanks, Jan Schlicht
Re: Review Request 36470: Updated scheduler driver to send TEARDOWN call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36470/ --- (Updated July 17, 2015, 3:18 a.m.) Review request for mesos and Ben Mahler. Changes --- benm's. NNFR. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs (updated) - src/sched/sched.cpp de76803581d32d7f7e93aac1678e3a2eba577e73 src/tests/exception_tests.cpp 9af16748ae67671bd327a1ea7c946a7a38c5f7ff src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e src/tests/master_allocator_tests.cpp 534b2486a6a02ef32b5a7235d3ad30e82a78d6a5 src/tests/master_tests.cpp 767c86cbde31eeb49d110d04bfb5a3eb5d469afc src/tests/rate_limiting_tests.cpp 49d907b0be45ccfed8af5c8fda89ad560e012c1e src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 Diff: https://reviews.apache.org/r/36470/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36518: Fixed a bug in master to properly handle resubscription.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36518/#review91977 --- Patch looks great! Reviews applied: [36560, 36518] All tests passed. - Mesos ReviewBot On July 16, 2015, 9:48 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36518/ --- (Updated July 16, 2015, 9:48 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3055 https://issues.apache.org/jira/browse/MESOS-3055 Repository: mesos Description --- In the process of fixing this, added an additional check in Master::registerFramework() that should've been there in the first place. Similar check exists in Master::reregisterFramework(). Diffs - src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 Diff: https://reviews.apache.org/r/36518/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36560: Made Subscribe.force optional in the Call protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36560/#review91971 --- Ship it! include/mesos/scheduler/scheduler.proto (lines 175 - 186) https://reviews.apache.org/r/36560/#comment145746 Hm.. this seems more confusing, why not just say in the beginning that this is only relevant for frameworks that have been assigned an ID? e.g. // If frameowrk_info.id is present, 'force' tells the master ... - Ben Mahler On July 16, 2015, 9:48 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36560/ --- (Updated July 16, 2015, 9:48 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3055 https://issues.apache.org/jira/browse/MESOS-3055 Repository: mesos Description --- Made 'force' optional because it doesn't make sense when subscribing without an id. Diffs - include/mesos/scheduler/scheduler.proto a027da255563c620fa3d7355ad47aa16d2264f77 src/examples/event_call_framework.cpp 17fdcac44c0a51293a318ef5184f4d48a461abd9 src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 Diff: https://reviews.apache.org/r/36560/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34136: Add ContainerImage protobuf.
On July 14, 2015, 9:03 p.m., Jiang Yan Xu wrote: include/mesos/mesos.proto, lines 1211-1213 https://reviews.apache.org/r/34136/diff/3/?file=1009139#file1009139line1211 So I found the use of the field `id` inconsistent in the code. Sometimes `id` has the `sha512-` prefix and sometimes not. I think we should consistently refer to `id` using the definition in the [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id), i.e., with the prefix. The fact that the ID is computed by the image creator using sha512 and that the provisioner validates it using sha512 is merely an implementation detail that is not a conern of higher level abstractions / APIs. So here I think in the comments we should not call it Image hash but rather refer to the spec for its full definition. We can of course call out the fact that it should have the sha512- perfix. What do you think? Timothy Chen wrote: Hi there, I'm going to commit this for Ian and just saw your comment. How about I reword the comment here to // The ID of the Image. Please refer to the Appc spec for its definition.? Timothy Chen wrote: Actually I mis-read what you meant, how about: // The ID of the Image. // An image ID is canonically represented as a string prefixed by // the algorithm used and the hash output (e.g. sha512-a83...). Jiang Yan Xu wrote: Or we could just copy the definition here verbatim. :) https://github.com/appc/spec/blob/master/spec/types.md#image-id-type An image ID is a string of the format hash-value, where hash is the hash algorithm used and value is the hex encoded string of the digest. Currently the only permitted hash algorithm is sha512. Thanks, please commit it! Thanks! - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34136/#review91666 --- On July 12, 2015, 4:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34136/ --- (Updated July 12, 2015, 4:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Add ContainerImage protobuf. Diffs - include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 Diff: https://reviews.apache.org/r/34136/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36461: Added FutureMessageType, DropMessagesType and ExpectNoFutureMessagesType.
On July 15, 2015, 5:29 p.m., Ben Mahler wrote: It looks like this could benefit from a bit of documentation that mentions the protobuf [union technique](https://developers.google.com/protocol-buffers/docs/techniques#union). added a comment on top of the matcher. On July 15, 2015, 5:29 p.m., Ben Mahler wrote: 3rdparty/libprocess/include/process/gmock.hpp, line 319 https://reviews.apache.org/r/36461/diff/1/?file=1010275#file1010275line319 Anything preventing s/t/type/ ? Type in this context is a bit confusing since it sounds like the message type. Can we call this something like 'UnionMessageMatcher' and avoid using the overloaded word type? s/MessageTypeMatcher/UnionMessageMatcher/ s/t/unionType/ On July 15, 2015, 5:29 p.m., Ben Mahler wrote: 3rdparty/libprocess/include/process/gmock.hpp, line 322 https://reviews.apache.org/r/36461/diff/1/?file=1010275#file1010275line322 Anything preventing s/n/message/? s/n/message/ - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36461/#review91757 --- On July 13, 2015, 11:55 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36461/ --- (Updated July 13, 2015, 11:55 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- Needed these abstractions for capturing specific CALLs explicitly in subsequent reviews. Diffs - 3rdparty/libprocess/include/process/gmock.hpp 0fd3f8cf06c9efd357fa7c933cc28d527855ac9a Diff: https://reviews.apache.org/r/36461/diff/ Testing --- Tested in subsequent reviews. Thanks, Vinod Kone
Re: Review Request 36462: Added FUTURE_CALL, DROP_CALL, DROP_CALLS and EXPECT_NO_FUTURE_CALLS.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36462/ --- (Updated July 17, 2015, 12:49 a.m.) Review request for mesos and Ben Mahler. Changes --- benm's comments. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- Needed these abstractions for capturing specific CALLs explicitly in subsequent reviews. Diffs (updated) - src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/36462/diff/ Testing --- Tested in subsequent reviews. Thanks, Vinod Kone
Re: Review Request 36461: Added FutureMessageType, DropMessagesType and ExpectNoFutureMessagesType.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36461/ --- (Updated July 17, 2015, 12:48 a.m.) Review request for mesos and Ben Mahler. Changes --- benm's comments. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- Needed these abstractions for capturing specific CALLs explicitly in subsequent reviews. Diffs (updated) - 3rdparty/libprocess/include/process/gmock.hpp e8781610f636954b39611fcb1de310a78ceea7cb Diff: https://reviews.apache.org/r/36461/diff/ Testing --- Tested in subsequent reviews. Thanks, Vinod Kone
Re: Review Request 36462: Added FUTURE_CALL, DROP_CALL, DROP_CALLS and EXPECT_NO_FUTURE_CALLS.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36462/#review92003 --- Ship it! Ship It! - Ben Mahler On July 17, 2015, 12:49 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36462/ --- (Updated July 17, 2015, 12:49 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- Needed these abstractions for capturing specific CALLs explicitly in subsequent reviews. Diffs - src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/36462/diff/ Testing --- Tested in subsequent reviews. Thanks, Vinod Kone
Re: Review Request 36464: Updated scheduler driver to send ACCEPT call.
On July 15, 2015, 5:58 p.m., Ben Mahler wrote: src/sched/sched.cpp, lines 1037-1050 https://reviews.apache.org/r/36464/diff/1/?file=1010281#file1010281line1037 Mind pulling this patch out into a separate review? Seems independent :) i'll just committ this as a standalone patch. ok with you? - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36464/#review91764 --- On July 13, 2015, 11:59 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36464/ --- (Updated July 13, 2015, 11:59 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 Diff: https://reviews.apache.org/r/36464/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36464: Updated scheduler driver to send ACCEPT call.
On July 15, 2015, 5:58 p.m., Ben Mahler wrote: src/sched/sched.cpp, lines 1037-1050 https://reviews.apache.org/r/36464/diff/1/?file=1010281#file1010281line1037 Mind pulling this patch out into a separate review? Seems independent :) Vinod Kone wrote: i'll just committ this as a standalone patch. ok with you? yep - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36464/#review91764 --- On July 13, 2015, 11:59 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36464/ --- (Updated July 13, 2015, 11:59 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 Diff: https://reviews.apache.org/r/36464/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36498: Implemented the OFFERS Event handler in the scheduler driver.
On July 16, 2015, 6:52 p.m., Vinod Kone wrote: src/sched/sched.cpp, lines 494-495 https://reviews.apache.org/r/36498/diff/1/?file=1011994#file1011994line494 couldn't you have used ANY which is the default? I only want to remove from the front and the end, ANY will remove from anywhere in the string. I'll use strings::trim instead, where ANY means front and back as you expected (although ANY is a bit confusing in this context :)). - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36498/#review91939 --- On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36498/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 and MESOS-3012 https://issues.apache.org/jira/browse/MESOS-2910 https://issues.apache.org/jira/browse/MESOS-3012 Repository: mesos Description --- This relies on 'Offer.url' to compute the pids needed for the message passing optimization (see [MESOS-3012](https://issues.apache.org/jira/browse/MESOS-3012)). Diffs - src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36498/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 34136: Add ContainerImage protobuf.
On July 14, 2015, 2:03 p.m., Jiang Yan Xu wrote: include/mesos/mesos.proto, lines 1211-1213 https://reviews.apache.org/r/34136/diff/3/?file=1009139#file1009139line1211 So I found the use of the field `id` inconsistent in the code. Sometimes `id` has the `sha512-` prefix and sometimes not. I think we should consistently refer to `id` using the definition in the [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id), i.e., with the prefix. The fact that the ID is computed by the image creator using sha512 and that the provisioner validates it using sha512 is merely an implementation detail that is not a conern of higher level abstractions / APIs. So here I think in the comments we should not call it Image hash but rather refer to the spec for its full definition. We can of course call out the fact that it should have the sha512- perfix. What do you think? Timothy Chen wrote: Hi there, I'm going to commit this for Ian and just saw your comment. How about I reword the comment here to // The ID of the Image. Please refer to the Appc spec for its definition.? Timothy Chen wrote: Actually I mis-read what you meant, how about: // The ID of the Image. // An image ID is canonically represented as a string prefixed by // the algorithm used and the hash output (e.g. sha512-a83...). Or we could just copy the definition here verbatim. :) https://github.com/appc/spec/blob/master/spec/types.md#image-id-type An image ID is a string of the format hash-value, where hash is the hash algorithm used and value is the hex encoded string of the digest. Currently the only permitted hash algorithm is sha512. Thanks, please commit it! - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34136/#review91666 --- On July 11, 2015, 9:47 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34136/ --- (Updated July 11, 2015, 9:47 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Add ContainerImage protobuf. Diffs - include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 Diff: https://reviews.apache.org/r/34136/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36461: Added FutureMessageType, DropMessagesType and ExpectNoFutureMessagesType.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36461/#review92006 --- Ship it! Ship It! - Ben Mahler On July 17, 2015, 12:48 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36461/ --- (Updated July 17, 2015, 12:48 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- Needed these abstractions for capturing specific CALLs explicitly in subsequent reviews. Diffs - 3rdparty/libprocess/include/process/gmock.hpp e8781610f636954b39611fcb1de310a78ceea7cb Diff: https://reviews.apache.org/r/36461/diff/ Testing --- Tested in subsequent reviews. Thanks, Vinod Kone
Re: Review Request 34136: Add ContainerImage protobuf.
On July 14, 2015, 9:03 p.m., Jiang Yan Xu wrote: include/mesos/mesos.proto, lines 1211-1213 https://reviews.apache.org/r/34136/diff/3/?file=1009139#file1009139line1211 So I found the use of the field `id` inconsistent in the code. Sometimes `id` has the `sha512-` prefix and sometimes not. I think we should consistently refer to `id` using the definition in the [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id), i.e., with the prefix. The fact that the ID is computed by the image creator using sha512 and that the provisioner validates it using sha512 is merely an implementation detail that is not a conern of higher level abstractions / APIs. So here I think in the comments we should not call it Image hash but rather refer to the spec for its full definition. We can of course call out the fact that it should have the sha512- perfix. What do you think? Timothy Chen wrote: Hi there, I'm going to commit this for Ian and just saw your comment. How about I reword the comment here to // The ID of the Image. Please refer to the Appc spec for its definition.? Actually I mis-read what you meant, how about: // The ID of the Image. // An image ID is canonically represented as a string prefixed by // the algorithm used and the hash output (e.g. sha512-a83...). - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34136/#review91666 --- On July 12, 2015, 4:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34136/ --- (Updated July 12, 2015, 4:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Add ContainerImage protobuf. Diffs - include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 Diff: https://reviews.apache.org/r/34136/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34137: Add support for container image provisioners.
On May 28, 2015, 9:49 p.m., Paul Brett wrote: src/slave/containerizer/mesos/containerizer.hpp, line 245 https://reviews.apache.org/r/34137/diff/1/?file=957263#file957263line245 To many underscores - can we come up with a better name? We can refactor later. - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review85624 --- On July 12, 2015, 4:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 12, 2015, 4:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36465: Updated scheduler driver to send REVIVE call.
On July 15, 2015, 5:58 p.m., Ben Mahler wrote: src/sched/sched.cpp, line 1029 https://reviews.apache.org/r/36465/diff/1/?file=1010283#file1010283line1029 newline before setting the type, like your other reviews? i'm doing new line after setting type. i'll make sure it's consistent. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36465/#review91765 --- On July 14, 2015, midnight, Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36465/ --- (Updated July 14, 2015, midnight) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d Diff: https://reviews.apache.org/r/36465/diff/ Testing --- make check (NOTE: There is already an existing test that tests the revive workflow, but it didn't need any update) Thanks, Vinod Kone
Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/ --- (Updated July 16, 2015, 9:32 p.m.) Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang. Changes --- Removed the dependency on 36424 Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version. Diffs (updated) - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/36378/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36488: Added oversubscription user doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/ --- (Updated July 16, 2015, 3:39 p.m.) Review request for mesos, Adam B and Jie Yu. Repository: mesos Description --- Added first draft of oversubscription user doc Diffs (updated) - docs/images/oversubscription-overview.jpg PRE-CREATION docs/oversubscription.md PRE-CREATION Diff: https://reviews.apache.org/r/36488/diff/ Testing --- Rendered at: https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md Thanks, Niklas Nielsen
Re: Review Request 36488: Added oversubscription user doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/#review91984 --- Bad patch! Reviews applied: [36488] Failed command: ./support/apply-review.sh -n -r 36488 Error: 2015-07-16 22:52:15 URL:https://reviews.apache.org/r/36488/diff/raw/ [10352/10352] - 36488.patch [1] error: missing binary patch data for 'docs/images/oversubscription-overview.jpg' error: binary patch does not apply to 'docs/images/oversubscription-overview.jpg' error: docs/images/oversubscription-overview.jpg: patch does not apply Failed to apply patch - Mesos ReviewBot On July 16, 2015, 10:39 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/ --- (Updated July 16, 2015, 10:39 p.m.) Review request for mesos, Adam B and Jie Yu. Repository: mesos Description --- Added first draft of oversubscription user doc Diffs - docs/images/oversubscription-overview.jpg PRE-CREATION docs/oversubscription.md PRE-CREATION Diff: https://reviews.apache.org/r/36488/diff/ Testing --- Rendered at: https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md Thanks, Niklas Nielsen
Review Request 36562: Store MasterInfo instead of UPID in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36562/ --- Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See https://reviews.apache.org/r/36497/ for context. Diffs - src/sched/sched.cpp 839fcbf0169fecf74fb17cab94fe4d35a1e20a10 Diff: https://reviews.apache.org/r/36562/diff/ Testing --- make check Thanks, Ben Mahler
Re: Review Request 34137: Add support for container image provisioners.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review92000 --- Tim, I probably wound wait for Vinod's shipit before committing this. There seems to be a bug in the code as well (besides the obvious broken build issue). src/slave/containerizer/mesos/containerizer.cpp (line 629) https://reviews.apache.org/r/34137/#comment145846 Hum, looks like a bug since, for example, slaveId is a reference and will be invalid when the lambda is called. In general, I think we should avoid using [=] for lambdas because its dangeous! I would prefer we resort to our old style defer style (e.g., introduce `_provision`). src/slave/containerizer/mesos/containerizer.cpp (lines 636 - 642) https://reviews.apache.org/r/34137/#comment145843 No test for checkpointing??? src/slave/containerizer/mesos/containerizer.cpp (line 1236) https://reviews.apache.org/r/34137/#comment145848 `_destroy` is a void function. We should do: ``` _destroy(...); return; ``` src/slave/containerizer/mesos/containerizer.cpp (line 1247) https://reviews.apache.org/r/34137/#comment145849 Ditto. src/slave/containerizer/mesos/containerizer.cpp (line 1268) https://reviews.apache.org/r/34137/#comment145850 s/cleanup/future/ src/slave/containerizer/provisioner.hpp (lines 37 - 38) https://reviews.apache.org/r/34137/#comment145836 This should be #include mesos/slave/isolator.hpp src/slave/containerizer/provisioner.hpp (lines 52 - 54) https://reviews.apache.org/r/34137/#comment145837 Recover containers? What container? This is a little misleading. How about ``` Recover root filesystems for containers... src/slave/containerizer/provisioner.cpp (line 24) https://reviews.apache.org/r/34137/#comment145840 Where is this? This won't compile! - Jie Yu On July 12, 2015, 4:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 12, 2015, 4:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36497: Implemented the SUBSCRIBE Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36497/ --- (Updated July 17, 2015, 1:36 a.m.) Review request for mesos and Vinod Kone. Changes --- Split apart the test. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- This one is non-trivial, note that we follow MESOS-786 with the exception of the 3rd case, since it is not possible and schedulers could not have possibly relied on getting registered instead of re-registered in that case. We now need to store the MasterInfo coming from the detector, as it is not provided in Event. Diffs (updated) - src/sched/sched.cpp 839fcbf0169fecf74fb17cab94fe4d35a1e20a10 src/tests/scheduler_event_call_tests.cpp cf6aa19a644580ff9d805e919763e9342d72677f Diff: https://reviews.apache.org/r/36497/diff/ Testing --- Added a test. Thanks, Ben Mahler
