Re: Review Request 63342: Fixed CMake binary dependencies.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63342/#review189398
---



Bad patch!

Reviews applied: [63342, 63341, 63340, 60628, 60626, 60624, 60623, 60622, 
60621, 60620]

Failed command: python support/apply-reviews.py -n -r 63342

Error:
2017-10-27 05:33:00 URL:https://reviews.apache.org/r/63342/diff/raw/ 
[2375/2375] -> "63342.patch" [1]
error: patch failed: src/tests/CMakeLists.txt:260
error: src/tests/CMakeLists.txt: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/19909/console

- Mesos Reviewbot


On Oct. 26, 2017, 10:54 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63342/
> ---
> 
> (Updated Oct. 26, 2017, 10:54 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, and Joseph Wu.
> 
> 
> Bugs: MESOS-8035
> https://issues.apache.org/jira/browse/MESOS-8035
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This resolves MESOS-8035 so that building just the `mesos-agent`, etc.
> target should correctly build its runtime dependencies (such as the
> containerizer, executor, etc.).
> 
> 
> Diffs
> -
> 
>   src/local/CMakeLists.txt 5e5bee3b70b2407bd86d9e7ffe0d169346054942 
>   src/slave/CMakeLists.txt 6f08f3dc95f3a1408cbef7b8f0b0cc2522558924 
>   src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 
> 
> 
> Diff: https://reviews.apache.org/r/63342/diff/1/
> 
> 
> Testing
> ---
> 
> Cleaned build folder, built just the `mesos-agent` target on Windows and 
> Linux, verified it could launch and connect to a master (built clean with 
> just `--target mesos-master`. Launched a simple task via Marathon to confirm 
> end-to-end support.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63279: Increased check tests task resources for Windows.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63279/#review189392
---



Patch looks great!

Reviews applied: [63268, 63269, 63271, 63272, 63273, 63274, 63275, 63276, 
63277, 63278, 63279]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Oct. 26, 2017, 4:40 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63279/
> ---
> 
> (Updated Oct. 26, 2017, 4:40 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Alexander Rukletsov, Jeff Coffler, 
> Gaston Kleiman, Jie Yu, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-6690
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Unfortunately, due to PowerShell's resource usage as a .NET program, on
> Windows 32 MB is not enough memory to run these tests. One instance of
> PowerShell takes > 128 MB, and two instances take > 256 MB.
> Realistically, the safe minimum is 512 MB of memory.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d3ffc0b0204d021f1ed734ece189d0e3a3fd8844 
> 
> 
> Diff: https://reviews.apache.org/r/63279/diff/1/
> 
> 
> Testing
> ---
> 
> Built and ran `mesos-tests.exe` on Windows repeatedly, verified all tests 
> still pass consistently (likewise for `stout` and `libprocess` tests.
> 
> ~~Currently verifying no breaks on Linux.~~ Built and ran tests on Linux.
> 
> NOTE: There are more check tests that are currently disabled for Windows, 
> that I think should be enabled, but did immediately work for me, so I've left 
> them disabled to unblock myself. Similarly, I would ideally like to port the 
> balloon example framework, and use that to prove the effectiveness of the job 
> object memory hard-cap. Having not yet ported it though, I manually verified 
> the effectiveness of the new isolators by launching test CPU and memory test 
> tasks on a deployed cluster my these changes (and the fact that PowerShell 
> OOM'ed was a nice verification too).
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 62997: Added checkpoint and recover capability for layers in provisioner.

[Zhitao Li]

> On Oct. 26, 2017, 11:45 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/provisioner/provisioner.hpp
> > Lines 170 (patched)
> > 
> >
> > why is it an `Option`?

Similar to `ContainerConfig`, we might be recovering backends launched before 
this patch?


- Zhitao


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62997/#review189378
---


On Oct. 17, 2017, 5:04 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62997/
> ---
> 
> (Updated Oct. 17, 2017, 5:04 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8079
> https://issues.apache.org/jira/browse/MESOS-8079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added checkpoint and recover capability for layers in provisioner.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 689acfcbbb07f071b6195472118a7a7520a44abd 
>   src/slave/containerizer/mesos/provisioner/paths.hpp 
> 466f5edab40732b0d8da4252a71fde9c2956e8e9 
>   src/slave/containerizer/mesos/provisioner/paths.cpp 
> 268dbeb4b18374ef53bc73254bf20ce6830e384f 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 7cba54ce490d1e6e17081cd7e04fd6759ceddb8e 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 450a3b32d69d2882973a6ed4e94e169a0256056b 
> 
> 
> Diff: https://reviews.apache.org/r/62997/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in Mesos containerizer.

[Gilbert Song]

> On Oct. 26, 2017, 12:33 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1113-1120 (original), 1152-1164 (patched)
> > 
> >
> > After looking at the slave code. it seems to me we can remove the whole 
> > part and the TODO.
> > 
> > Not yours. this was from a tech debt when we combined the launch() 
> > method 
> > https://github.com/apache/mesos/commit/17ffb97ae7eda78edf85b204eba35bc59649a479
> 
> Zhitao Li wrote:
> Are you suggesting that a nested container's config does not need 
> `executor_info` anymore?

no, we already set it in slave.cpp before calling `containerizer::launch()`.


- Gilbert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/#review188981
---


On Oct. 19, 2017, 9:41 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> ---
> 
> (Updated Oct. 19, 2017, 9:41 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate whether 
> recovered.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 78fdd21f8b7ede4beedff31ba2b488ffebd4ea31 
>   src/slave/containerizer/mesos/paths.hpp 
> a03f15e01e8eb8a1326baad8d7db96a79d785482 
>   src/slave/containerizer/mesos/paths.cpp 
> 0c61c20c345a327ec469b382558aaeed0280e754 
> 
> 
> Diff: https://reviews.apache.org/r/55334/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 62997: Added checkpoint and recover capability for layers in provisioner.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62997/#review189378
---




src/slave/containerizer/mesos/provisioner/provisioner.hpp
Lines 170 (patched)


why is it an `Option`?



src/slave/containerizer/mesos/provisioner/provisioner.cpp
Lines 415-417 (patched)


fix the indentation. two spaces to the right.



src/slave/containerizer/mesos/provisioner/provisioner.cpp
Line 469 (original), 520 (patched)


Should we set the `layers`?


- Gilbert Song


On Oct. 17, 2017, 10:04 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62997/
> ---
> 
> (Updated Oct. 17, 2017, 10:04 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8079
> https://issues.apache.org/jira/browse/MESOS-8079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added checkpoint and recover capability for layers in provisioner.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 689acfcbbb07f071b6195472118a7a7520a44abd 
>   src/slave/containerizer/mesos/provisioner/paths.hpp 
> 466f5edab40732b0d8da4252a71fde9c2956e8e9 
>   src/slave/containerizer/mesos/provisioner/paths.cpp 
> 268dbeb4b18374ef53bc73254bf20ce6830e384f 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 7cba54ce490d1e6e17081cd7e04fd6759ceddb8e 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 450a3b32d69d2882973a6ed4e94e169a0256056b 
> 
> 
> Diff: https://reviews.apache.org/r/62997/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 63314: Add support for Ubuntu 16.04 in docker build.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63314/#review189376
---



Patch looks great!

Reviews applied: [63314]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Oct. 26, 2017, 8:08 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63314/
> ---
> 
> (Updated Oct. 26, 2017, 8:08 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for Ubuntu 16.04 in docker build.
> 
> 
> Diffs
> -
> 
>   support/docker-build.sh 50e693ca71d499f71f2935923f4691feef560b12 
> 
> 
> Diff: https://reviews.apache.org/r/63314/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 63094: Added resource version uuid for offer operations.

[Greg Mann]

> On Oct. 18, 2017, 9:46 a.m., Benjamin Bannier wrote:
> > include/mesos/resource_provider/resource_provider.proto
> > Lines 56-66 (patched)
> > 
> >
> > I would prefer if we would not assume that this number changes for 
> > _every_ resource change, but just for failures.
> > * * *
> > 
> > For one we do not want users to assume that this number has certain 
> > values so we can recognize failures. Imagine a user applied an operation to 
> > the resource provider resources (`sequence_id=0`), and the agent speculated 
> > that the new `sequence_id` would be `1`. Now the operation fails in the RP 
> > and its `sequence_id` would be incremented to `1`. We cannot distinguish a 
> > case where the agent assumed the operation succeeded from a case where it 
> > failed based on the clock value alone in this case. Would we just increase 
> > this number in case of failures it would be clear that any operation 
> > assuming `sequence_id=0` would be invalid after the failure since the agent 
> > would be unlikely to assume failures on a "normal" execution path. 
> > 
> > * * *
> > 
> > Another reason would be that it would be great to be able to use 
> > similar notions of `sequence_id` for the agent's view of its RPs, and the 
> > master's view of its agents. We plan to implement speculative application 
> > of "legacy" offer operations like e.g., `RESERVE`, `CREATE` in the master, 
> > and incrementing this number even in successful cases would require too 
> > much knowledge about how the agent applies these operations in the master.
> > 
> > Imagine an agent with resource provider resources `disk(*):1` and 
> > `disk(*):1` (each `disk` from a different resource provider). The agent 
> > will expose `disk(*):2` to the master. A framework now reserves, 
> > `disk(*):2->disk(A):2`. The agent would inform the resource providers about 
> > that operation. Let's say the first of these operations succeeds, but the 
> > second on fails (the argument works as well for the other order, or even 
> > concurrent application). Having the master increment this 
> > `resource_sequence_id` for speculated operations would require it to know 
> > that the single `RESERVE` operation did map onto two state changes, i.e., 
> > it should have incremented its agent `resource_sequence_id` by two instead 
> > of just by one.
> > 
> > This seems not only leaky, but also inflexible since it becomes hard to 
> > decompose an agent's resources without affecting the master's assumptions.
> > 
> > * * *
> > 
> > Having `sequence_id` just increase in case of failures would eliminate 
> > a large class of cases where users would need to make assumption about how 
> > it does change. Instead we would more clearly move into a direction where 
> > it is only changed by pushes from lower layers (where an operation either 
> > succeeded or failed) to user layers above. This should also simplify how we 
> > would project multiple `sequence_id`s onto a single `sequence_id` (multiple 
> > resource provider `sequence_id`s onto agent `sequence_id`); in that case we 
> > could just take the maximum of all RP IDs and the agent ID (e.g., last 
> > failure of an operation on any RP is equivalent to last failure of any 
> > operation on the agent). Counting all operations makes this harder, 
> > especially when thinking of master-speculated operations.
> 
> Greg Mann wrote:
> This confused me a bit: "Having the master increment this 
> resource_sequence_id for speculated operations"
> 
> IIUC, the master will only increment the sequence ID when it receives a 
> {Register,Reregister,Update}SlaveMessage from the agent containing a new ID. 
> So, I don't think the ID is incremented routinely during the successful 
> application of offer operations.
> 
> My understanding of the comment in this patch is that the sequence ID 
> will be incremented only when a legacy operation fails, or when the RP has 
> its total resources updated (i.e., storage backend has more/less storage to 
> offer and updates the CSI plugin/RP accordingly).
> 
> Jie Yu wrote:
> I had some offline discussion with Benjamin. It looks to us that using a 
> single scalar clock value for the agent does not make sense. It's more easy 
> to reason about the correctness if we maintain per RP resource version uuid. 
> I updated this review with the new thinking. PTAL.
> 
> Jie Yu wrote:
> See this notes to see why a single scalar clock for the agnet does not 
> work:
> 
> https://docs.google.com/document/d/1RrrLVATZUyaURpEOeGjgxA6ccshuLo94G678IbL-Yco/edit#bookmark=id.v893d0oct2wh

After some discussion, I'm fine with using a vector clock.


- Greg


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63094/#review188465

Re: Review Request 62938: Added ZooKeeper leader resolution to CLI.

[Mesos Reviewbot Windows]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62938/#review189372
---



PASS: Mesos patch 62938 was successfully built and tested.

Reviews applied: `['62938']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62938

- Mesos Reviewbot Windows


On Oct. 26, 2017, 4:52 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62938/
> ---
> 
> (Updated Oct. 26, 2017, 4:52 p.m.)
> 
> 
> Review request for mesos, Eric Chung and Kevin Klues.
> 
> 
> Bugs: MESOS-8012
> https://issues.apache.org/jira/browse/MESOS-8012
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change allows users to use the CLI with a Mesos running in high
> availability mode. The `zookeeper` field was already here before this
> commit, with an `addresses` array and a `path` field. This change
> only adds the backend to actually make it usable.
> 
> Interacting with ZooKeeper requires a new dependency, kazoo, that has
> been added to the list of requirements for the CLI.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/README.md a4b270d9b54cdd83bd62530fb44ddfaffb8a014b 
>   src/python/cli_new/lib/cli/util.py dd109c09368e650b7d7dabd663c11bc2e3e5180a 
>   src/python/cli_new/pip-requirements.txt 
> 7aeac344c47ccd2588fded44d7314db7abd85653 
> 
> 
> Diff: https://reviews.apache.org/r/62938/diff/2/
> 
> 
> Testing
> ---
> 
> Tested with a Mesos cluster running with 3 masters and 1 agent, run some CLI 
> commands successfully.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 63094: Added resource version uuid for offer operations.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63094/#review189371
---




include/mesos/v1/resource_provider/resource_provider.proto
Lines 116 (patched)


s/The resource version UUID is used/Used/

Here and elsewhere.



src/messages/messages.proto
Lines 608 (patched)


s/Describe/Describes/



src/messages/messages.proto
Lines 712 (patched)


Do we need multiple ResourceVersionUUIDs here? Can the master just include 
the UUID for the relevant resource provider/agent?


- Greg Mann


On Oct. 19, 2017, 9:35 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63094/
> ---
> 
> (Updated Oct. 19, 2017, 9:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Chun-Hung Hsiao, 
> Gaston Kleiman, Greg Mann, Joseph Wu, Jan Schlicht, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The resource version UUID is used to establish the relationship
> between the operation and the resources that the operation is
> operating on. Each resource provider will keep a resource version
> UUID, and changes it when it believes that the resources from this
> resource provider are out of sync from the master's view.  The master
> will keep track of the last known resource version UUID for each
> resource provider, and attach the resource version UUID in each
> operation it sends out. The resource provider should reject operations
> that have a different resource version UUID than that it maintains,
> because this means the operation is operating on resources that might
> have already been invalidated.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/resource_provider.proto 
> f5a9073075327019fd133bd51265f695ef464845 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> e5cbede5b6e57a8641fca1ebfee5454f292cc24c 
>   src/messages/messages.proto 0a32b3457e9143a7d48670610ca3e56dd516136f 
>   src/tests/resource_provider_manager_tests.cpp 
> ca49e1f0203494fc8b4a4507c33e5a3885a14a59 
>   src/tests/resource_provider_validation_tests.cpp 
> f182bff4670318e9de22c2915c5dbb423a74ad6c 
> 
> 
> Diff: https://reviews.apache.org/r/63094/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63340: Moved Java build code to `java/CMakeLists.txt`.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63340/
---

(Updated Oct. 26, 2017, 3:55 p.m.)


Review request for mesos, Jeff Coffler, John Kordich, and Joseph Wu.


Changes
---

Add dependency (I moved `mesos-fetcher` which #60628 enabled).


Repository: mesos


Description
---

Moved Java build code to `java/CMakeLists.txt`.


Diffs
-

  src/CMakeLists.txt 219252f6f82b2d62d024b2ab876fa0ba2f5c8e6c 
  src/java/CMakeLists.txt PRE-CREATION 


Diff: https://reviews.apache.org/r/63340/diff/1/


Testing
---

Built with `-DENABLE_JAVA=ON` and attached Marathon framework to `master` 
(which requries Java bindings). Also built and ran `mesos-tests` with the 
ZooKeeper unit tests (which use the Java components).

Had to add `mesos-protobufs` as a dependency to `mesos-jar` to be totally 
correct.


Thanks,

Andrew Schwartzmeyer

Review Request 63342: Fixed CMake binary dependencies.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63342/
---

Review request for mesos, Jeff Coffler, John Kordich, and Joseph Wu.


Bugs: MESOS-8035
https://issues.apache.org/jira/browse/MESOS-8035


Repository: mesos


Description
---

This resolves MESOS-8035 so that building just the `mesos-agent`, etc.
target should correctly build its runtime dependencies (such as the
containerizer, executor, etc.).


Diffs
-

  src/local/CMakeLists.txt 5e5bee3b70b2407bd86d9e7ffe0d169346054942 
  src/slave/CMakeLists.txt 6f08f3dc95f3a1408cbef7b8f0b0cc2522558924 
  src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 


Diff: https://reviews.apache.org/r/63342/diff/1/


Testing
---

Cleaned build folder, built just the `mesos-agent` target on Windows and Linux, 
verified it could launch and connect to a master (built clean with just 
`--target mesos-master`. Launched a simple task via Marathon to confirm 
end-to-end support.


Thanks,

Andrew Schwartzmeyer

Review Request 63341: Set `BUILD_FLAGS` flag in CMake.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63341/
---

Review request for mesos, Jeff Coffler, John Kordich, and Joseph Wu.


Bugs: MESOS-5455
https://issues.apache.org/jira/browse/MESOS-5455


Repository: mesos


Description
---

This resolves MESOS-5455, and consolidates the `BUILD` variables into
one location.


Diffs
-

  cmake/CompilationConfigure.cmake 929e45bd810c99e036d433a8f0a0bc978c9841a2 
  src/CMakeLists.txt 219252f6f82b2d62d024b2ab876fa0ba2f5c8e6c 
  src/common/build_config.hpp.in ac7059ab2aa54011197b9e1a46f9d7fd39ac39c6 


Diff: https://reviews.apache.org/r/63341/diff/1/


Testing
---

Checked the emitted `build_config.hpp` under various conditions; flags are set 
correctly (and can still build of course).


Thanks,

Andrew Schwartzmeyer

Review Request 63340: Moved Java build code to `java/CMakeLists.txt`.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63340/
---

Review request for mesos, Jeff Coffler, John Kordich, and Joseph Wu.


Repository: mesos


Description
---

Moved Java build code to `java/CMakeLists.txt`.


Diffs
-

  src/CMakeLists.txt 219252f6f82b2d62d024b2ab876fa0ba2f5c8e6c 
  src/java/CMakeLists.txt PRE-CREATION 


Diff: https://reviews.apache.org/r/63340/diff/1/


Testing
---

Built with `-DENABLE_JAVA=ON` and attached Marathon framework to `master` 
(which requries Java bindings). Also built and ran `mesos-tests` with the 
ZooKeeper unit tests (which use the Java components).

Had to add `mesos-protobufs` as a dependency to `mesos-jar` to be totally 
correct.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 63001: Updated protobuf definitions related to offer operations.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63001/#review189354
---




include/mesos/mesos.proto
Lines 2157 (patched)


s/status/statuses/



include/mesos/mesos.proto
Lines 2157-2158 (patched)


Can we clarify here that this is an ordered list?



include/mesos/mesos.proto
Lines 2162 (patched)


s/framework specified/framework-specified/

s/id/ID/ ??

Here and below.



include/mesos/mesos.proto
Lines 2204 (patched)


Backticks instead of quotes here for consistency.



include/mesos/resource_provider/resource_provider.proto
Line 66 (original), 72 (patched)


Do you want to use `OFFER_OPERATION` here instead? Or, do you think it's OK 
to just use `OPERATION` since this is within the RP? Here and elsewhere.



include/mesos/v1/mesos.proto
Lines 2145 (patched)


I'm a little bit concerned that the names of the various IDs in this RR 
will become confusing for devs. We have:
1) `OfferOperationID`, with a field name of `operation_id`
2) `bytes uuid`, with a field name of `uuid` (this is the status update ID)
3) `bytes uuid`, with a field name of `operation_uuid` (this is our 
internal ID for the operation)

What would you think about naming the fields `operation_id`, 
`operation_update_uuid`, and `operation_internal_uuid`, respectively? (could 
also just do `update_uuid` and `internal_uuid` for the latter two)

Since the fields are in different messages, I'm not sure how bad this would 
really be, just a thought. Let me know what you think.



include/mesos/v1/mesos.proto
Lines 2185 (patched)


Backticks instead of quotes here for consistency.



src/messages/messages.proto
Lines 608-610 (original), 608-610 (patched)


Add a note about offer operations here?



src/messages/messages.proto
Lines 638 (patched)


s/update/updates/


- Greg Mann


On Oct. 19, 2017, 12:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63001/
> ---
> 
> (Updated Oct. 19, 2017, 12:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Gaston Kleiman, 
> Greg Mann, Jan Schlicht, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated protobuf definitions related to offer operations.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 859fdff4d9a0604bc506b08af79075084ae23466 
>   include/mesos/resource_provider/resource_provider.proto 
> f5a9073075327019fd133bd51265f695ef464845 
>   include/mesos/v1/mesos.proto cfd4abd3af1d8c9fbd31659161eada9ec9f92282 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> e5cbede5b6e57a8641fca1ebfee5454f292cc24c 
>   src/messages/messages.proto 0a32b3457e9143a7d48670610ca3e56dd516136f 
>   src/resource_provider/manager.cpp 31fcb789f5ab907511e868c374c49f7457a33ed3 
>   src/resource_provider/validation.cpp 
> d2927227f60ab0d4ae2481ad73a31ee444b48ee0 
>   src/tests/resource_provider_validation_tests.cpp 
> f182bff4670318e9de22c2915c5dbb423a74ad6c 
> 
> 
> Diff: https://reviews.apache.org/r/63001/diff/9/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63001: Updated protobuf definitions related to offer operations.

[Greg Mann]

> On Oct. 16, 2017, 6:33 p.m., Greg Mann wrote:
> > src/messages/messages.proto
> > Lines 637-638 (patched)
> > 
> >
> > I think that this list should include pending operations, but should 
> > _not_ include terminal operations with unacknowledged updates.
> > 
> > When the master receives an update, it needs to decide whether or not 
> > it should update the allocator. If the operation was pending when 
> > `UpdateSlaveMessage` was sent, then the master should update the allocator. 
> > If the operation was not pending but its update had not been acknowledged, 
> > then the result of that operation was already reflected in the last 
> > `UpdateSlaveMessage` from the agent, and the master should not update the 
> > allocator.
> > 
> > If the agent includes only pending operations in this field, then the 
> > master will be able to make this distinction.
> 
> Jie Yu wrote:
> The reason we need to send terminal but unacked operations is for 
> reconciliation. Master always keeps a cache about all the known non-completed 
> operations (either pending or terminal but unacked). This information can be 
> used to answer reconciliation requests.
> 
> The master can distinguish between a pending operation vs. a terminal but 
> unacked operation by looking at latest state of the operation. The master 
> update the allocator if the latest state if terminal, irrespective whether 
> it's acked or not.
> 
> That does bring up an issue. Consider the following two cases:
> 
> 1) Master knows about an operation that the agent does not know about. 
> This can happen if the operation gets lost on its way to the agent. This will 
> trigger an agent re-registration. The master in this case will need to 
> generate a reconcile message (similar to that in `reconcileKnownSlave`) so 
> that the agent or LRP can reply with a terminal status update. This will 
> guarantee that the resources allocated for this operation are properly 
> recovered to allocator.
> 
> This case is also possible if there is a speculation failure on old 
> operations and result in an `UpdateSlaveMessage` being sent to the master. 
> The operation that master knows about (but agent does not) will eventually 
> reach agent or LRP, and will be rejected by the LRP or the agent. The master 
> will properly recovered the resources to the allocator when a failed update 
> is received.
> 
> 2) Agent knows about an operation that the master does not know about. 
> For instance, This is a new master that just fails over, and an LRP 
> re-registers with the agent. In that case, the master needs to update the 
> 'allocated' (i.e., 'used') resources for those new operations. This is 
> similar to what we do when slave registers. However, the tricky part is that 
> the allocator does not have an interface currently allowing us to add more 
> allocation to a given agent. We likely need to add that (or make it part of 
> `Allocator::updateSlave` interface).
> 
> Greg Mann wrote:
> OK, I see why we need to include unacked updates as well. Thanks Jie!
> 
> Greg Mann wrote:
> Hey Jie, I have another question related to this issue so I re-opened it 
> for discussion.
> 
> Previously you wrote "The master update the allocator if the latest state 
> if terminal, irrespective whether it's acked or not."
> This implies that subsequent retries of the update will _not_ have the 
> `latest_state` set; otherwise, the master would not be able to distinguish 
> between the first terminal status update and subsequent retries. Was it your 
> intention to suggest that behavior? Looking at the `Slave::forward` helper 
> that we use when intitializing the status update manager, it looks like we 
> _always_ set `latest_state` for task status updates: 
> https://github.com/apache/mesos/blob/f26ffcee0a359a644968feca1ec91243401f589a/src/slave/slave.cpp#L4867-L4870
> 
> I suppose we could set the latest state the first time an operation 
> update is sent to master, and then _not_ set it in subsequent retries. Then 
> the value of `.has_latest_state()` would be a direct indication of whether or 
> not an update is a retry.

Ah, I see that `latest_status` is optional, so I suspect this was indeed your 
intention. Just want to confirm :)


- Greg


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63001/#review188162
---


On Oct. 19, 2017, 12:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63001/
> ---
> 
> (Updated Oct. 19, 2017, 12:08 a.m.)
> 
> 
> Review request for mesos, Benjamin 

Re: Review Request 63001: Updated protobuf definitions related to offer operations.

[Greg Mann]

> On Oct. 16, 2017, 6:33 p.m., Greg Mann wrote:
> > src/messages/messages.proto
> > Lines 637-638 (patched)
> > 
> >
> > I think that this list should include pending operations, but should 
> > _not_ include terminal operations with unacknowledged updates.
> > 
> > When the master receives an update, it needs to decide whether or not 
> > it should update the allocator. If the operation was pending when 
> > `UpdateSlaveMessage` was sent, then the master should update the allocator. 
> > If the operation was not pending but its update had not been acknowledged, 
> > then the result of that operation was already reflected in the last 
> > `UpdateSlaveMessage` from the agent, and the master should not update the 
> > allocator.
> > 
> > If the agent includes only pending operations in this field, then the 
> > master will be able to make this distinction.
> 
> Jie Yu wrote:
> The reason we need to send terminal but unacked operations is for 
> reconciliation. Master always keeps a cache about all the known non-completed 
> operations (either pending or terminal but unacked). This information can be 
> used to answer reconciliation requests.
> 
> The master can distinguish between a pending operation vs. a terminal but 
> unacked operation by looking at latest state of the operation. The master 
> update the allocator if the latest state if terminal, irrespective whether 
> it's acked or not.
> 
> That does bring up an issue. Consider the following two cases:
> 
> 1) Master knows about an operation that the agent does not know about. 
> This can happen if the operation gets lost on its way to the agent. This will 
> trigger an agent re-registration. The master in this case will need to 
> generate a reconcile message (similar to that in `reconcileKnownSlave`) so 
> that the agent or LRP can reply with a terminal status update. This will 
> guarantee that the resources allocated for this operation are properly 
> recovered to allocator.
> 
> This case is also possible if there is a speculation failure on old 
> operations and result in an `UpdateSlaveMessage` being sent to the master. 
> The operation that master knows about (but agent does not) will eventually 
> reach agent or LRP, and will be rejected by the LRP or the agent. The master 
> will properly recovered the resources to the allocator when a failed update 
> is received.
> 
> 2) Agent knows about an operation that the master does not know about. 
> For instance, This is a new master that just fails over, and an LRP 
> re-registers with the agent. In that case, the master needs to update the 
> 'allocated' (i.e., 'used') resources for those new operations. This is 
> similar to what we do when slave registers. However, the tricky part is that 
> the allocator does not have an interface currently allowing us to add more 
> allocation to a given agent. We likely need to add that (or make it part of 
> `Allocator::updateSlave` interface).
> 
> Greg Mann wrote:
> OK, I see why we need to include unacked updates as well. Thanks Jie!

Hey Jie, I have another question related to this issue so I re-opened it for 
discussion.

Previously you wrote "The master update the allocator if the latest state if 
terminal, irrespective whether it's acked or not."
This implies that subsequent retries of the update will _not_ have the 
`latest_state` set; otherwise, the master would not be able to distinguish 
between the first terminal status update and subsequent retries. Was it your 
intention to suggest that behavior? Looking at the `Slave::forward` helper that 
we use when intitializing the status update manager, it looks like we _always_ 
set `latest_state` for task status updates: 
https://github.com/apache/mesos/blob/f26ffcee0a359a644968feca1ec91243401f589a/src/slave/slave.cpp#L4867-L4870

I suppose we could set the latest state the first time an operation update is 
sent to master, and then _not_ set it in subsequent retries. Then the value of 
`.has_latest_state()` would be a direct indication of whether or not an update 
is a retry.


- Greg


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63001/#review188162
---


On Oct. 19, 2017, 12:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63001/
> ---
> 
> (Updated Oct. 19, 2017, 12:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Gaston Kleiman, 
> Greg Mann, Jan Schlicht, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated protobuf definitions related to offer 

Re: Review Request 63279: Increased check tests task resources for Windows.

[Jeff Coffler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63279/#review189363
---




src/tests/check_tests.cpp
Line 80 (original), 80 (patched)


Nit: Please fix commit message:

`Realistically, the safe mimimum is 512 MB of.`

Of what? (Memory, obviously, but please fix.)


- Jeff Coffler


On Oct. 26, 2017, 4:40 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63279/
> ---
> 
> (Updated Oct. 26, 2017, 4:40 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Alexander Rukletsov, Jeff Coffler, 
> Gaston Kleiman, Jie Yu, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-6690
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Unfortunately, due to PowerShell's resource usage as a .NET program, on
> Windows 32 MB is not enough memory to run these tests. One instance of
> PowerShell takes > 128 MB, and two instances take > 256 MB.
> Realistically, the safe minimum is 512 MB of.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d3ffc0b0204d021f1ed734ece189d0e3a3fd8844 
> 
> 
> Diff: https://reviews.apache.org/r/63279/diff/1/
> 
> 
> Testing
> ---
> 
> Built and ran `mesos-tests.exe` on Windows repeatedly, verified all tests 
> still pass consistently (likewise for `stout` and `libprocess` tests.
> 
> ~~Currently verifying no breaks on Linux.~~ Built and ran tests on Linux.
> 
> NOTE: There are more check tests that are currently disabled for Windows, 
> that I think should be enabled, but did immediately work for me, so I've left 
> them disabled to unblock myself. Similarly, I would ideally like to port the 
> balloon example framework, and use that to prove the effectiveness of the job 
> object memory hard-cap. Having not yet ported it though, I manually verified 
> the effectiveness of the new isolators by launching test CPU and memory test 
> tasks on a deployed cluster my these changes (and the fact that PowerShell 
> OOM'ed was a nice verification too).
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63279: Increased check tests task resources for Windows.

[Jeff Coffler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63279/#review189362
---


Ship it!




Ship It!

- Jeff Coffler


On Oct. 26, 2017, 4:40 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63279/
> ---
> 
> (Updated Oct. 26, 2017, 4:40 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Alexander Rukletsov, Jeff Coffler, 
> Gaston Kleiman, Jie Yu, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-6690
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Unfortunately, due to PowerShell's resource usage as a .NET program, on
> Windows 32 MB is not enough memory to run these tests. One instance of
> PowerShell takes > 128 MB, and two instances take > 256 MB.
> Realistically, the safe minimum is 512 MB of.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d3ffc0b0204d021f1ed734ece189d0e3a3fd8844 
> 
> 
> Diff: https://reviews.apache.org/r/63279/diff/1/
> 
> 
> Testing
> ---
> 
> Built and ran `mesos-tests.exe` on Windows repeatedly, verified all tests 
> still pass consistently (likewise for `stout` and `libprocess` tests.
> 
> ~~Currently verifying no breaks on Linux.~~ Built and ran tests on Linux.
> 
> NOTE: There are more check tests that are currently disabled for Windows, 
> that I think should be enabled, but did immediately work for me, so I've left 
> them disabled to unblock myself. Similarly, I would ideally like to port the 
> balloon example framework, and use that to prove the effectiveness of the job 
> object memory hard-cap. Having not yet ported it though, I manually verified 
> the effectiveness of the new isolators by launching test CPU and memory test 
> tasks on a deployed cluster my these changes (and the fact that PowerShell 
> OOM'ed was a nice verification too).
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63277: Windows: Ported CPU and memory isolator tests.

[Andrew Schwartzmeyer]

> On Oct. 26, 2017, 2:59 p.m., Jeff Coffler wrote:
> > src/tests/containerizer/cpu_isolator_tests.cpp
> > Lines 120 (patched)
> > 
> >
> > Given what we learned about quoting, why not just do:
> > 
> > "powershell -c while ($true) {}"
> > 
> > ?

Actually, this should be `-NoProfile -Command` too, and `"` is fine (needed to 
avoid interpolation by `cmd.exe`), it's `'` that's troublesome.


> On Oct. 26, 2017, 2:59 p.m., Jeff Coffler wrote:
> > src/tests/containerizer/cpu_isolator_tests.cpp
> > Line 163 (original), 172 (patched)
> > 
> >
> > A powershell loop wouldn't do it. `Get-Process` is a step in the right 
> > direction, but probably has a small amount of kernel time and a lot of 
> > PowerShell time for formatting, etc.
> > 
> > What about some sort of tight loop that was essentially exclusively a 
> > kernel operation, like mapping and unmapping a memory segment in a loop or 
> > something? This could be done in C, avoids PowerShell entirely, and would 
> > need to be processed by the kernel. Or creating a shared memory mutex, or 
> > anything along those lines.
> > 
> > The kernel is supposed to be efficient, so it would be hard to consume 
> > Kernel time, but pick something that you know is 100% kernel, and do it in 
> > a tight loop. That should get you there.

This needs to be a task that can be launched. I really do not want to have to 
ship yet another binary just for a test. I can if I have to, but there should 
be a way to accomplish this outside of that.


> On Oct. 26, 2017, 2:59 p.m., Jeff Coffler wrote:
> > src/tests/containerizer/cpu_isolator_tests.cpp
> > Lines 224 (patched)
> > 
> >
> > Ditto

This needs to be wrapped in `"` because of the `|`.


> On Oct. 26, 2017, 2:59 p.m., Jeff Coffler wrote:
> > src/tests/containerizer/cpu_isolator_tests.cpp
> > Line 234 (original), 253 (patched)
> > 
> >
> > If you're doing a `sleep` operation, that won't consume system time. 
> > The kernel will deschedule you until it's time for you to wake up.
> > 
> > You might get there by doing a lot of short sleeps, but that's 
> > generally risky and unreliable (depends on kernel timing, speed of 
> > implementation, etc).

The `sleep` here applies to the test process, not the executing task.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63277/#review189358
---


On Oct. 26, 2017, 9:33 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63277/
> ---
> 
> (Updated Oct. 26, 2017, 9:33 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
> Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-6690
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These test the limitation and usage reporting of the new Windows
> `Cpuset` and `Mem` isolators.
> 
> 
> Diffs
> -
> 
>   src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 
>   src/tests/containerizer/cpu_isolator_tests.cpp 
> 846b2e255547a02f5eb0590747edca62bc560ac3 
>   src/tests/containerizer/memory_isolator_tests.cpp 
> b8ea5d35b3a0a4820d9ec4c6d7d916dc6101b570 
>   src/tests/mesos.cpp 9185b5bf2175be5b0f6b6a03a04e9e5445bf22fd 
> 
> 
> Diff: https://reviews.apache.org/r/63277/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63278: Windows: Documented the `cpuset` and `mem` isolators.

[Jeff Coffler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63278/#review189360
---


Ship it!




Ship It!

- Jeff Coffler


On Oct. 26, 2017, 4:34 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63278/
> ---
> 
> (Updated Oct. 26, 2017, 4:34 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
> Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-6690
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds documentation on the usage of the job object isolators, which
> enable task limits, as well as the statistics they report.
> 
> 
> Diffs
> -
> 
>   docs/isolators/windows.md PRE-CREATION 
>   docs/mesos-containerizer.md 9cb071273a5ce42784457ea01b363911fdb7773d 
> 
> 
> Diff: https://reviews.apache.org/r/63278/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63277: Windows: Ported CPU and memory isolator tests.

[Jeff Coffler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63277/#review189358
---




src/tests/containerizer/cpu_isolator_tests.cpp
Lines 120 (patched)


Given what we learned about quoting, why not just do:

"powershell -c while ($true) {}"

?



src/tests/containerizer/cpu_isolator_tests.cpp
Line 163 (original), 172 (patched)


A powershell loop wouldn't do it. `Get-Process` is a step in the right 
direction, but probably has a small amount of kernel time and a lot of 
PowerShell time for formatting, etc.

What about some sort of tight loop that was essentially exclusively a 
kernel operation, like mapping and unmapping a memory segment in a loop or 
something? This could be done in C, avoids PowerShell entirely, and would need 
to be processed by the kernel. Or creating a shared memory mutex, or anything 
along those lines.

The kernel is supposed to be efficient, so it would be hard to consume 
Kernel time, but pick something that you know is 100% kernel, and do it in a 
tight loop. That should get you there.



src/tests/containerizer/cpu_isolator_tests.cpp
Lines 224 (patched)


Ditto



src/tests/containerizer/cpu_isolator_tests.cpp
Line 234 (original), 253 (patched)


If you're doing a `sleep` operation, that won't consume system time. The 
kernel will deschedule you until it's time for you to wake up.

You might get there by doing a lot of short sleeps, but that's generally 
risky and unreliable (depends on kernel timing, speed of implementation, etc).


- Jeff Coffler


On Oct. 26, 2017, 4:33 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63277/
> ---
> 
> (Updated Oct. 26, 2017, 4:33 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
> Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-6690
> https://issues.apache.org/jira/browse/MESOS-6690
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These test the limitation and usage reporting of the new Windows
> `Cpuset` and `Mem` isolators.
> 
> 
> Diffs
> -
> 
>   src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 
>   src/tests/containerizer/cpu_isolator_tests.cpp 
> 846b2e255547a02f5eb0590747edca62bc560ac3 
>   src/tests/containerizer/memory_isolator_tests.cpp 
> b8ea5d35b3a0a4820d9ec4c6d7d916dc6101b570 
>   src/tests/mesos.cpp 9185b5bf2175be5b0f6b6a03a04e9e5445bf22fd 
> 
> 
> Diff: https://reviews.apache.org/r/63277/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63275: Windows: Abstracted out `os::name_job` in stout.

[Jeff Coffler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63275/#review189355
---


Ship it!




Ship It!

- Jeff Coffler


On Oct. 26, 2017, 4:31 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63275/
> ---
> 
> (Updated Oct. 26, 2017, 4:31 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all uses identify a job object by the PID, but the job object
> handle must be obtained by name. So this overloads `os::open_job` to
> call `os::name_job` when given a PID.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
> 
> 
> Diff: https://reviews.apache.org/r/63275/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 59687: Added tests for recovering ContainerConfig.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59687/#review189351
---


Fix it, then Ship it!





src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 1773 (patched)


newline above.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 1777 (patched)


newline above



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 1788 (patched)


newline below.


- Gilbert Song


On Sept. 26, 2017, 11:14 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59687/
> ---
> 
> (Updated Sept. 26, 2017, 11:14 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jason Lai.
> 
> 
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for recovering ContainerConfig.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> f8b4423de8a468501336acc5ee0c67f181dc65f5 
> 
> 
> Diff: https://reviews.apache.org/r/59687/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 63312: Updated Resources::apply for new operations.

[Mesos Reviewbot Windows]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63312/#review189350
---



FAIL: Failed to apply the current review.

Failed command: `python.exe .\support\apply-reviews.py -n -r 63312`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63312

Relevant logs:

- 
[apply-review-63312-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63312/logs/apply-review-63312-stdout.log):

```
error: patch failed: src/v1/resources.cpp:1814
error: src/v1/resources.cpp: patch does not apply
```

- Mesos Reviewbot Windows


On Oct. 25, 2017, 2:16 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63312/
> ---
> 
> (Updated Oct. 25, 2017, 2:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and 
> Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updated Resources::apply method to support new operations:
> CREATE_VOLUME, DESTROY_VOLUME, CREATE_BLOCK and DESTROY_BLOCK. We
> introduced an optional `convertedResources` field to this method for
> new operations.
> 
> This patch also made sure that we don't call Resources::apply for
> LAUNCH and LAUNCH_GROUP, because it does not really "convert" the
> resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 9f959494f5c0376aa4ebd8f2571f4b1f45c83cb3 
>   include/mesos/v1/resources.hpp a6216858136c82d5ebb5da3bb22c0b8a58a3b75d 
>   src/common/resources.cpp 7ee4dae1389e037531aec533a3d235ee06443ea8 
>   src/examples/persistent_volume_framework.cpp 
> 1f8f4bea7ce4d1a919034e0639b4f70e7c9961cf 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5b6efe5faa3c3b10f1f714f582a155b368f8ccaf 
>   src/v1/resources.cpp 5c0a196cf87b377b345e53a36ed24727d52381ca 
> 
> 
> Diff: https://reviews.apache.org/r/63312/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63315: Allowe override of parallel jobs in docker build.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63315/#review189349
---



Patch looks great!

Reviews applied: [63315]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Oct. 25, 2017, 9:58 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63315/
> ---
> 
> (Updated Oct. 25, 2017, 9:58 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use JOBS env variable to control `make -j` parameter.
> By default `JOBS=6`.
> 
> 
> Diffs
> -
> 
>   support/docker-build.sh 50e693ca71d499f71f2935923f4691feef560b12 
> 
> 
> Diff: https://reviews.apache.org/r/63315/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 63271: Windows: Added `os::set_job_memory_limit` to stout.

[Jeff Coffler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63271/#review189347
---


Ship it!




Ship It!

- Jeff Coffler


On Oct. 26, 2017, 4:29 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63271/
> ---
> 
> (Updated Oct. 26, 2017, 4:29 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is used to set a hard cap on the memory usage for a job object.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
> 
> 
> Diff: https://reviews.apache.org/r/63271/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63273: Windows: Added `os::get_job_processes` to stout.

[Jeff Coffler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63273/#review189346
---


Ship it!




Ship It!

- Jeff Coffler


On Oct. 26, 2017, 4:31 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63273/
> ---
> 
> (Updated Oct. 26, 2017, 4:31 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Returns a `set` for all the processes in a job object.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
> 
> 
> Diff: https://reviews.apache.org/r/63273/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 55335: Backfilled required fields in TaskInfo in MesosContainerizer* tests.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55335/#review189345
---




src/tests/containerizer/mesos_containerizer_tests.cpp
Lines 103-119 (patched)


neither introduce a helper here nor a new helper in test/mesos.hpp, let's 
call createTask() and pass in an empty resource.


- Gilbert Song


On Sept. 26, 2017, 1:14 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55335/
> ---
> 
> (Updated Sept. 26, 2017, 1:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These tests were using a `TaskInfo` field which is missing required
> protobuf fields. After the previous patch begins to checkpoint
> `ContainerConfig`, the incomplete `TaskInfo` cannot be checkpointed,
> thus caused these tests to fail.
> 
> This patch backfills these fields to make these tests pass.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> e61a85df6ec5308ccd2832e66df803b0ad7b53ee 
> 
> 
> Diff: https://reviews.apache.org/r/55335/diff/6/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="MesosContainerizer*" make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 63271: Windows: Added `os::set_job_memory_limit` to stout.

[Andrew Schwartzmeyer]

> On Oct. 26, 2017, 1:55 p.m., Jeff Coffler wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp
> > Lines 784 (patched)
> > 
> >
> > I usually see NOLINT on a line of it's own. Is that a mistake, making 
> > the line even longer with `NOLINT`?

As far as I can tell from grepping through the code, it's always at the end of 
the line that should not be linted. There's one or two places it's by itself, 
and those are probably wrong.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63271/#review189342
---


On Oct. 26, 2017, 9:29 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63271/
> ---
> 
> (Updated Oct. 26, 2017, 9:29 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is used to set a hard cap on the memory usage for a job object.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
> 
> 
> Diff: https://reviews.apache.org/r/63271/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63272: Windows: Added `os::get_job_info` to stout.

[Jeff Coffler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63272/#review189343
---




3rdparty/stout/include/stout/windows/os.hpp
Lines 730 (patched)


This line is > 80 bytes, needs `NOLINT` added.


- Jeff Coffler


On Oct. 26, 2017, 4:30 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63272/
> ---
> 
> (Updated Oct. 26, 2017, 4:30 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This returns a `JOBOBJECT_BASIC_ACCOUNTING_INFORMATION`, which can be
> inspected for basic CPU / process accounting information for a group of
> processes within a job object.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
> 
> 
> Diff: https://reviews.apache.org/r/63272/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63271: Windows: Added `os::set_job_memory_limit` to stout.

[Jeff Coffler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63271/#review189342
---




3rdparty/stout/include/stout/windows/os.hpp
Lines 784 (patched)


I usually see NOLINT on a line of it's own. Is that a mistake, making the 
line even longer with `NOLINT`?


- Jeff Coffler


On Oct. 26, 2017, 4:29 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63271/
> ---
> 
> (Updated Oct. 26, 2017, 4:29 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is used to set a hard cap on the memory usage for a job object.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
> 
> 
> Diff: https://reviews.apache.org/r/63271/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63268: Windows: Fixed variable casing in `windows/os.hpp`.

[Jeff Coffler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63268/#review189340
---


Ship it!




Ship It!

- Jeff Coffler


On Oct. 26, 2017, 4:27 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63268/
> ---
> 
> (Updated Oct. 26, 2017, 4:27 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes the CamelCase to snake_case per the style guide for stout.
> It also adds `::` to uses of `::GetLastError` where it was missing, and
> includes `` because it was used but missing.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
> 
> 
> Diff: https://reviews.apache.org/r/63268/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in Mesos containerizer.

[Zhitao Li]

> On Oct. 26, 2017, 7:33 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1113-1120 (original), 1152-1164 (patched)
> > 
> >
> > After looking at the slave code. it seems to me we can remove the whole 
> > part and the TODO.
> > 
> > Not yours. this was from a tech debt when we combined the launch() 
> > method 
> > https://github.com/apache/mesos/commit/17ffb97ae7eda78edf85b204eba35bc59649a479

Are you suggesting that a nested container's config does not need 
`executor_info` anymore?


- Zhitao


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/#review188981
---


On Oct. 20, 2017, 4:41 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> ---
> 
> (Updated Oct. 20, 2017, 4:41 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate whether 
> recovered.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 78fdd21f8b7ede4beedff31ba2b488ffebd4ea31 
>   src/slave/containerizer/mesos/paths.hpp 
> a03f15e01e8eb8a1326baad8d7db96a79d785482 
>   src/slave/containerizer/mesos/paths.cpp 
> 0c61c20c345a327ec469b382558aaeed0280e754 
> 
> 
> Diff: https://reviews.apache.org/r/55334/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 55335: Backfilled required fields in TaskInfo in MesosContainerizer* tests.

[Gilbert Song]

> On Sept. 25, 2017, 6:23 p.m., Gilbert Song wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp
> > Line 714 (original), 714-720 (patched)
> > 
> >
> > will it look cleaner if use `createTask()` helper?
> 
> Zhitao Li wrote:
> I created a local helper function. All `createTask()` helpers requires 
> some `resources` to work which is not really applicable right now.

we can leverage it by using zero resources.


- Gilbert


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55335/#review186191
---


On Sept. 26, 2017, 1:14 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55335/
> ---
> 
> (Updated Sept. 26, 2017, 1:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These tests were using a `TaskInfo` field which is missing required
> protobuf fields. After the previous patch begins to checkpoint
> `ContainerConfig`, the incomplete `TaskInfo` cannot be checkpointed,
> thus caused these tests to fail.
> 
> This patch backfills these fields to make these tests pass.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> e61a85df6ec5308ccd2832e66df803b0ad7b53ee 
> 
> 
> Diff: https://reviews.apache.org/r/55335/diff/6/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="MesosContainerizer*" make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 63269: Windows: Added `os::set_job_cpu_limit` to stout.

[Mesos Reviewbot Windows]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63269/#review189334
---



PASS: Mesos patch 63269 was successfully built and tested.

Reviews applied: `['63268', '63269']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63269

- Mesos Reviewbot Windows


On Oct. 26, 2017, 4:29 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63269/
> ---
> 
> (Updated Oct. 26, 2017, 4:29 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is used to set a hard cap on the CPU usage for a job object.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
> 
> 
> Diff: https://reviews.apache.org/r/63269/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in Mesos containerizer.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/#review188981
---


Fix it, then Ship it!





src/slave/containerizer/mesos/containerizer.cpp
Lines 757-758 (patched)


we don't quote the containerID since it was generated by the agent or 
executor, not by users.



src/slave/containerizer/mesos/containerizer.cpp
Lines 764-765 (patched)


ditto.



src/slave/containerizer/mesos/containerizer.cpp
Lines 764-765 (patched)


how about:
```
  LOG(INFO) << "No container config is recovered for container " << 
containerId
<< ", image pruning will be disabled.";
```



src/slave/containerizer/mesos/containerizer.cpp
Lines 765 (patched)


we don't have a period at the end for `LOG(INFO)`.



src/slave/containerizer/mesos/containerizer.cpp
Line 793 (original), 811 (patched)


not yours. mind add the containerID here?



src/slave/containerizer/mesos/containerizer.cpp
Lines 819 (patched)


add containerID?



src/slave/containerizer/mesos/containerizer.cpp
Lines 823 (patched)


do we need to mention image pruning will be disabled?



src/slave/containerizer/mesos/containerizer.cpp
Lines 860 (patched)


s/config/container config/g



src/slave/containerizer/mesos/containerizer.cpp
Lines 861 (patched)


remove `this means`.



src/slave/containerizer/mesos/containerizer.cpp
Lines 862 (patched)


ditto.



src/slave/containerizer/mesos/containerizer.cpp
Lines 1113-1120 (original), 1152-1164 (patched)


After looking at the slave code. it seems to me we can remove the whole 
part and the TODO.

Not yours. this was from a tech debt when we combined the launch() method 
https://github.com/apache/mesos/commit/17ffb97ae7eda78edf85b204eba35bc59649a479



src/slave/containerizer/mesos/containerizer.cpp
Lines 1163 (patched)


ditto.



src/slave/containerizer/mesos/containerizer.cpp
Lines 1271 (patched)


could we move it below `CHECK_EQ(container->state, PROVISIONING);`?



src/slave/containerizer/mesos/containerizer.cpp
Lines 1321 (patched)


good catch/



src/slave/containerizer/mesos/paths.cpp
Lines 279-280 (original), 282-283 (patched)


mind add the containerID?



src/slave/containerizer/mesos/paths.cpp
Lines 310-311 (patched)


ditto.



src/slave/containerizer/mesos/paths.cpp
Lines 467-468 (patched)


extra newline?


- Gilbert Song


On Oct. 19, 2017, 9:41 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> ---
> 
> (Updated Oct. 19, 2017, 9:41 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate whether 
> recovered.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 78fdd21f8b7ede4beedff31ba2b488ffebd4ea31 
>   src/slave/containerizer/mesos/paths.hpp 
> a03f15e01e8eb8a1326baad8d7db96a79d785482 
>   src/slave/containerizer/mesos/paths.cpp 
> 0c61c20c345a327ec469b382558aaeed0280e754 
> 
> 
> Diff: https://reviews.apache.org/r/55334/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 63332: Logged the resources in DRF sorter CHECK.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63332/#review189328
---


Ship it!




Ship It!

- Jiang Yan Xu


On Oct. 26, 2017, 12:01 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63332/
> ---
> 
> (Updated Oct. 26, 2017, 12:01 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If calls to these checks fails, log related resources objects which
> causes the failure.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 77e52dec735d276389643f7f356cd763b2f785e9 
> 
> 
> Diff: https://reviews.apache.org/r/63332/diff/5/
> 
> 
> Testing
> ---
> 
> Compile it on Linux.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 63332: Logged the resources in DRF sorter CHECK.

[Zhitao Li]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63332/
---

(Updated Oct. 26, 2017, 7:01 p.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


Changes
---

Fix compilation.


Repository: mesos


Description
---

If calls to these checks fails, log related resources objects which
causes the failure.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.hpp 
77e52dec735d276389643f7f356cd763b2f785e9 


Diff: https://reviews.apache.org/r/63332/diff/5/

Changes: https://reviews.apache.org/r/63332/diff/4-5/


Testing (updated)
---

Compile it on Linux.


Thanks,

Zhitao Li

Re: Review Request 63332: Logged the resources in DRF sorter CHECK.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63332/#review189327
---




src/master/allocator/sorter/drf/sorter.hpp
Lines 392 (patched)


Typo: s/toRemove/oldAllocation/

Could you make sure it compiles and fill out the "Testing Done" section?


- Jiang Yan Xu


On Oct. 26, 2017, 11:26 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63332/
> ---
> 
> (Updated Oct. 26, 2017, 11:26 a.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If calls to these checks fails, log related resources objects which
> causes the failure.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 77e52dec735d276389643f7f356cd763b2f785e9 
> 
> 
> Diff: https://reviews.apache.org/r/63332/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63253/#review189326
---



Patch looks great!

Reviews applied: [60620, 60621, 60622, 60623, 60624, 60626, 60628, 63253]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Oct. 25, 2017, 11:19 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63253/
> ---
> 
> (Updated Oct. 25, 2017, 11:19 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Gaston Kleiman, John Kordich, 
> Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017)
> for proper operation on Windows. Changed to use new URI methods
> introduced as part of MESOS-6705, and used powershell on Windows
> to replace "test" command on Linux.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 5078bd4d70698f5cbd14c971fcecfd58f8467a04 
> 
> 
> Diff: https://reviews.apache.org/r/63253/diff/4/
> 
> 
> Testing
> ---
> 
> Ran mesos-tests on both Windows and Linux with no errors.
> 
> Specifically ran mesos-tests with 
> `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the 
> modified tests ran properly.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 63322: Added placeholder handlers and other changes for operation updates.

[Mesos Reviewbot Windows]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63322/#review189325
---



PASS: Mesos patch 63322 was successfully built and tested.

Reviews applied: `['63001', '62903', '63094', '63321', '63322']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63322

- Mesos Reviewbot Windows


On Oct. 26, 2017, 7:01 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63322/
> ---
> 
> (Updated Oct. 26, 2017, 7:01 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-8130
> https://issues.apache.org/jira/browse/MESOS-8130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds empty placeholder handler functions which will
> be used for offer operation status updates as well as their
> acknowledgement and reconciliation.
> 
> A number of switch statements are also updated to handle new
> enum values and validation code is added.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp f5d4bc5da26f232a1fad1169b0c656b19132b853 
>   src/examples/long_lived_framework.cpp 
> adfe10eca2af26e8294171256e5f053fb46abf24 
>   src/examples/test_http_framework.cpp 
> e640800055bd9a946663de377970e867d29c750a 
>   src/master/http.cpp 42139bec519d36316e324ef921157c49cdf2d043 
>   src/master/master.hpp a3c9530e340b14b739da90851227f8eed254b4ac 
>   src/master/master.cpp 4b76648b4887724e166ff95e002ee26c8bfef006 
>   src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb 
>   src/sched/sched.cpp fb77b79a05d5db21c8642cb8915e9bdcc1e2b1de 
>   src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef 
>   src/slave/slave.cpp d8477b4e364270f5ad61311aa8fe48f823bc7aac 
>   src/tests/mesos.hpp 4b61f2dbd67a5e8a65a6655519c8f03e5d6c6954 
> 
> 
> Diff: https://reviews.apache.org/r/63322/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 63332: Logged the resources in DRF sorter CHECK.

[Zhitao Li]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63332/
---

(Updated Oct. 26, 2017, 6:26 p.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


Changes
---

Remove quotes around agent id.


Repository: mesos


Description
---

If calls to these checks fails, log related resources objects which
causes the failure.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.hpp 
77e52dec735d276389643f7f356cd763b2f785e9 


Diff: https://reviews.apache.org/r/63332/diff/4/

Changes: https://reviews.apache.org/r/63332/diff/3-4/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 63332: Logged the resources in DRF sorter CHECK.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63332/#review189324
---


Fix it, then Ship it!





src/master/allocator/sorter/drf/sorter.hpp
Lines 350 (patched)


Nit: For consistency, I don't think we quote agent IDs (there are no spaces 
in agent IDs).

Here and below?


- Jiang Yan Xu


On Oct. 26, 2017, 11:12 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63332/
> ---
> 
> (Updated Oct. 26, 2017, 11:12 a.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If calls to these checks fails, log related resources objects which
> causes the failure.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 77e52dec735d276389643f7f356cd763b2f785e9 
> 
> 
> Diff: https://reviews.apache.org/r/63332/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 63332: Logged the resources in DRF sorter CHECK.

[Zhitao Li]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63332/
---

(Updated Oct. 26, 2017, 6:12 p.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


Repository: mesos


Description
---

If calls to these checks fails, log related resources objects which
causes the failure.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.hpp 
77e52dec735d276389643f7f356cd763b2f785e9 


Diff: https://reviews.apache.org/r/63332/diff/3/

Changes: https://reviews.apache.org/r/63332/diff/2-3/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 63332: Logged the resources in DRF sorter CHECK.

[Zhitao Li]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63332/
---

(Updated Oct. 26, 2017, 6:10 p.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


Changes
---

Also log agent id if it's indexed.


Repository: mesos


Description
---

If calls to these checks fails, log related resources objects which
causes the failure.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.hpp 
77e52dec735d276389643f7f356cd763b2f785e9 


Diff: https://reviews.apache.org/r/63332/diff/2/

Changes: https://reviews.apache.org/r/63332/diff/1-2/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 63332: Logged the resources in DRF sorter CHECK.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63332/#review189323
---


Fix it, then Ship it!





src/master/allocator/sorter/drf/sorter.hpp
Lines 385-386 (original), 388-392 (patched)


Not yours, but can we use 

```
CHECK(resources.contains(slaveId));
CHECK(resources.at(slaveId).contains(oldAllocation)) << ...
```

like above?

It doesn't strictly belong to this review but since you are editing these 
lines we might as well.


- Jiang Yan Xu


On Oct. 26, 2017, 9:42 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63332/
> ---
> 
> (Updated Oct. 26, 2017, 9:42 a.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If calls to these checks fails, log related resources objects which
> causes the failure.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 77e52dec735d276389643f7f356cd763b2f785e9 
> 
> 
> Diff: https://reviews.apache.org/r/63332/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 62938: Added ZooKeeper leader resolution to CLI.

[Eric Chung]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62938/#review189321
---




src/python/cli_new/lib/cli/util.py
Lines 247-253 (patched)


why not just use list comprehension?



src/python/cli_new/lib/cli/util.py
Lines 257 (patched)


why not just use `if not json_data`?


- Eric Chung


On Oct. 26, 2017, 4:52 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62938/
> ---
> 
> (Updated Oct. 26, 2017, 4:52 p.m.)
> 
> 
> Review request for mesos, Eric Chung and Kevin Klues.
> 
> 
> Bugs: MESOS-8012
> https://issues.apache.org/jira/browse/MESOS-8012
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change allows users to use the CLI with a Mesos running in high
> availability mode. The `zookeeper` field was already here before this
> commit, with an `addresses` array and a `path` field. This change
> only adds the backend to actually make it usable.
> 
> Interacting with ZooKeeper requires a new dependency, kazoo, that has
> been added to the list of requirements for the CLI.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/README.md a4b270d9b54cdd83bd62530fb44ddfaffb8a014b 
>   src/python/cli_new/lib/cli/util.py dd109c09368e650b7d7dabd663c11bc2e3e5180a 
>   src/python/cli_new/pip-requirements.txt 
> 7aeac344c47ccd2588fded44d7314db7abd85653 
> 
> 
> Diff: https://reviews.apache.org/r/62938/diff/2/
> 
> 
> Testing
> ---
> 
> Tested with a Mesos cluster running with 3 masters and 1 agent, run some CLI 
> commands successfully.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62938: Added ZooKeeper leader resolution to CLI.

[Armand Grillet]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62938/
---

(Updated Oct. 26, 2017, 4:52 p.m.)


Review request for mesos, Eric Chung and Kevin Klues.


Changes
---

Sorted nodes.


Bugs: MESOS-8012
https://issues.apache.org/jira/browse/MESOS-8012


Repository: mesos


Description (updated)
---

This change allows users to use the CLI with a Mesos running in high
availability mode. The `zookeeper` field was already here before this
commit, with an `addresses` array and a `path` field. This change
only adds the backend to actually make it usable.

Interacting with ZooKeeper requires a new dependency, kazoo, that has
been added to the list of requirements for the CLI.


Diffs (updated)
-

  src/python/cli_new/README.md a4b270d9b54cdd83bd62530fb44ddfaffb8a014b 
  src/python/cli_new/lib/cli/util.py dd109c09368e650b7d7dabd663c11bc2e3e5180a 
  src/python/cli_new/pip-requirements.txt 
7aeac344c47ccd2588fded44d7314db7abd85653 


Diff: https://reviews.apache.org/r/62938/diff/2/

Changes: https://reviews.apache.org/r/62938/diff/1-2/


Testing
---

Tested with a Mesos cluster running with 3 masters and 1 agent, run some CLI 
commands successfully.


Thanks,

Armand Grillet

Review Request 63332: Logged the resources in DRF sorter CHECK.

[Zhitao Li]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63332/
---

Review request for mesos, James Peach and Jiang Yan Xu.


Repository: mesos


Description
---

If calls to these checks fails, log related resources objects which
causes the failure.


Diffs
-

  src/master/allocator/sorter/drf/sorter.hpp 
77e52dec735d276389643f7f356cd763b2f785e9 


Diff: https://reviews.apache.org/r/63332/diff/1/


Testing
---


Thanks,

Zhitao Li

Review Request 63279: Increased check tests task resources for Windows.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63279/
---

Review request for mesos, Akash Gupta, Alexander Rukletsov, Jeff Coffler, 
Gaston Kleiman, Jie Yu, John Kordich, Joseph Wu, and Li Li.


Bugs: MESOS-6690
https://issues.apache.org/jira/browse/MESOS-6690


Repository: mesos


Description
---

Unfortunately, due to PowerShell's resource usage as a .NET program, on
Windows 32 MB is not enough memory to run these tests. One instance of
PowerShell takes > 128 MB, and two instances take > 256 MB.
Realistically, the safe minimum is 512 MB of.


Diffs
-

  src/tests/check_tests.cpp d3ffc0b0204d021f1ed734ece189d0e3a3fd8844 


Diff: https://reviews.apache.org/r/63279/diff/1/


Testing
---

Built and ran `mesos-tests.exe` on Windows repeatedly, verified all tests still 
pass consistently (likewise for `stout` and `libprocess` tests.

Currently verifying no breaks on Linux.

NOTE: There are more check tests that are currently disabled for Windows, that 
I think should be enabled, but did immediately work for me, so I've left them 
disabled to unblock myself. Similarly, I would ideally like to port the balloon 
example framework, and use that to prove the effectiveness of the job object 
memory hard-cap. Having not yet ported it though, I manually verified the 
effectiveness of the new isolators by launching test CPU and memory test tasks 
on a deployed cluster my these changes (and the fact that PowerShell OOM'ed was 
a nice verification too).


Thanks,

Andrew Schwartzmeyer

Review Request 63278: Windows: Documented the `cpuset` and `mem` isolators.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63278/
---

Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
Joseph Wu, and Li Li.


Bugs: MESOS-6690
https://issues.apache.org/jira/browse/MESOS-6690


Repository: mesos


Description
---

This adds documentation on the usage of the job object isolators, which
enable task limits, as well sa the statistics they report.


Diffs
-

  docs/isolators/windows.md PRE-CREATION 
  docs/mesos-containerizer.md 9cb071273a5ce42784457ea01b363911fdb7773d 


Diff: https://reviews.apache.org/r/63278/diff/1/


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 63277: Windows: Ported CPU and memory isolator tests.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63277/
---

Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
Joseph Wu, and Li Li.


Bugs: MESOS-6690
https://issues.apache.org/jira/browse/MESOS-6690


Repository: mesos


Description
---

These test the limitation and usage reporting of the new Windows
`Cpuset` and `Mem` isolators.


Diffs
-

  src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 
  src/tests/containerizer/cpu_isolator_tests.cpp 
846b2e255547a02f5eb0590747edca62bc560ac3 
  src/tests/containerizer/memory_isolator_tests.cpp 
b8ea5d35b3a0a4820d9ec4c6d7d916dc6101b570 
  src/tests/mesos.cpp 9185b5bf2175be5b0f6b6a03a04e9e5445bf22fd 


Diff: https://reviews.apache.org/r/63277/diff/1/


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 63276: Windows: Added `Cpuset` and `Mem` isolators.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63276/
---

Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
Joseph Wu, and Li Li.


Bugs: MESOS-6690
https://issues.apache.org/jira/browse/MESOS-6690


Repository: mesos


Description
---

Instead of deriving from the POSIX isolators, we now have two real
Windows isolators that can be used together or separately. The `Cpuset`
isolator enables a hard-cap CPU limit, and the `Mem` isolator enables a
hard-cap memory limit on the job object for the container.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
100e3bbda543d87808da9ff6bea42da5099ea8c5 
  src/slave/containerizer/mesos/isolators/windows.hpp 
b0621a5fc411b8812722f7fcf6580ed64dac5382 
  src/slave/flags.cpp 789b45b8d84fc01733a885754204fe2fdd6d6807 


Diff: https://reviews.apache.org/r/63276/diff/1/


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 63273: Windows: Added `os::get_job_processes` to stout.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63273/
---

Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
Joseph Wu, and Michael Park.


Repository: mesos


Description
---

Returns a `set` for all the processes in a job object.


Diffs
-

  3rdparty/stout/include/stout/windows/os.hpp 
09ddec6d69472cd13b453fe1a77fdbe343fc23c8 


Diff: https://reviews.apache.org/r/63273/diff/1/


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 63274: Windows: Added `os::get_job_memory` to stout.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63274/
---

Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, 
Joseph Wu, and Michael Park.


Repository: mesos


Description
---

This function sums the working set size for each process in a job object
represented by the PID. The Windows API does not support this directly.
Instead, the list of processes in a job object must be obtained, and
then for each process, the memory usage must be queried.


Diffs
-

  3rdparty/stout/include/stout/windows/os.hpp 
09ddec6d69472cd13b453fe1a77fdbe343fc23c8 


Diff: https://reviews.apache.org/r/63274/diff/1/


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 63275: Windows: Abstracted out `os::name_job` in stout.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63275/
---

Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, and 
Joseph Wu.


Repository: mesos


Description
---

Almost all uses identify a job object by the PID, but the job object
handle must be obtained by name. So this overloads `os::open_job` to
call `os::name_job` when given a PID.


Diffs
-

  3rdparty/stout/include/stout/windows/os.hpp 
09ddec6d69472cd13b453fe1a77fdbe343fc23c8 


Diff: https://reviews.apache.org/r/63275/diff/1/


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 63272: Windows: Added `os::get_job_info` to stout.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63272/
---

Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, and 
Joseph Wu.


Repository: mesos


Description
---

This returns a `JOBOBJECT_BASIC_ACCOUNTING_INFORMATION`, which can be
inspected for basic CPU / process accounting information for a group of
processes within a job object.


Diffs
-

  3rdparty/stout/include/stout/windows/os.hpp 
09ddec6d69472cd13b453fe1a77fdbe343fc23c8 


Diff: https://reviews.apache.org/r/63272/diff/1/


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 63271: Windows: Added `os::set_job_memory_limit` to stout.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63271/
---

Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, and 
Joseph Wu.


Repository: mesos


Description
---

This is used to set a hard cap on the memory usage for a job object.


Diffs
-

  3rdparty/stout/include/stout/windows/os.hpp 
09ddec6d69472cd13b453fe1a77fdbe343fc23c8 


Diff: https://reviews.apache.org/r/63271/diff/1/


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 63269: Windows: Added `os::set_job_cpu_limit` to stout.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63269/
---

Review request for mesos, Akash Gupta, Jeff Coffler, Jie Yu, John Kordich, and 
Joseph Wu.


Repository: mesos


Description
---

This is used to set a hard cap on the CPU usage for a job object.


Diffs
-

  3rdparty/stout/include/stout/windows/os.hpp 
09ddec6d69472cd13b453fe1a77fdbe343fc23c8 


Diff: https://reviews.apache.org/r/63269/diff/1/


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 63268: Windows: Fixed variable casing in `windows/os.hpp`.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63268/
---

Review request for mesos, Jeff Coffler, Jie Yu, John Kordich, and Joseph Wu.


Repository: mesos


Description
---

This changes the CamelCase to snake_case per the style guide for stout.
It also adds `::` to uses of `::GetLastError` where it was missing, and
includes `` because it was used but missing.


Diffs
-

  3rdparty/stout/include/stout/windows/os.hpp 
09ddec6d69472cd13b453fe1a77fdbe343fc23c8 


Diff: https://reviews.apache.org/r/63268/diff/1/


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 63293: Update google-glog to 0.3.5.

[Andrew Schwartzmeyer]

> On Oct. 25, 2017, 6:43 p.m., Andrew Schwartzmeyer wrote:
> > Is there a pressing need to update this _right now_? As it is, we are 
> > waiting for 0.3.6 (MESOS-3394) so that we can converge on a single version 
> > of Glog for all platforms.
> 
> Tomasz Janiszewski wrote:
> Nope. We can wait for 0.3.6. We have merged config.guess that open a way 
> to build on ARM. I'll discard this PR. I'm not sure if our all issues will be 
> solved in 0.3.6. The glog patch contains fix for cmake build. I'm not sure if 
> glog 0.3.6 fixes this issue. 
> I'm going to close this PR.

Awesome. Let's try to make sure we get any fixes we need upstream before 0.3.6 
drops. Ideally we can drop any manual patching :)


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63293/#review189264
---


On Oct. 25, 2017, 5:28 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63293/
> ---
> 
> (Updated Oct. 25, 2017, 5:28 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3322
> https://issues.apache.org/jira/browse/MESOS-3322
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Version 0.3.5 adds support for PowerPC so no patching is required.
> 
> Fixes: MESOS-3322
> Refs: https://github.com/google/glog/releases/tag/v0.3.5
> 
> 
> Diffs
> -
> 
>   3rdparty/glog-0.3.5.patch ab15bea04d7b8af31a638b7c4ce3256ed2f0df4e 
> 
> 
> Diff: https://reviews.apache.org/r/63293/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 63245: Ported and enabled Os Tests on Windows.

[Raluca Miclea]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63245/
---

(Updated Oct. 26, 2017, 2:59 p.m.)


Review request for mesos and Andrew Schwartzmeyer.


Bugs: MESOS-3441
https://issues.apache.org/jira/browse/MESOS-3441


Repository: mesos


Description (updated)
---

TEST_F(OsTest, Libraries)
The test failed because no environmental variable was provided for
Windows. I used "PATH" as environmental variable on Windows since
there's no default variable where the linker should look into while
linking dynamic libraries/shared libraries. (like LD_LIBRARY_PATH on Unix/Linux)

TEST_F(OsTest, Sleep)
The test hanged because of the last assertion:
ASSERT_ERROR(os::sleep(Milliseconds(-10)));
Apparently, it didn't handle a negative sleep duration. By simply
returning an error value when the duration is negative the problem was
solved.

TEST_F(OsTest, SYMLINK_Size)
First issue was this assertion:
EXPECT_SOME_EQ(size, os::stat::size(file,FollowSymlink::DO_NOT_FOLLOW_SYMLINK));
It fails everytime because  is not a symbolic link. When calling
os:stat:size with argument  set to "DO_NOT_FOLLOW_SYMLINK", it firstly
checks whether the path it receives is a symlink in order to get the symlink 
data
for that path. If it's not a symlink, it returns an error.
If you are supposed to be able to test the size of a non-link file in
the context of not-following-a-link, the program should return the
size of the non-link file instead of returning an error upon determining the
nature of the path as not being a link file path. Right now, the assertion
is not relevant since you'll get an error if you supply it a non-link file
path rather thana symlink.

TEST_F_TEMP_DISABLED_ON_WINDOWS(OsTest, SYMLINK_Realpath)
The issue was that the symlink resolved to itself but it was tested
against the path of the target file. Once the path of the target file
was replaced by the path of the symlink, the test passed.


Diffs (updated)
-

  3rdparty/stout/include/stout/os.hpp 1a81db61faa55d7043d75a012fe1e66b49bf601c 
  3rdparty/stout/include/stout/os/windows/stat.hpp 
0db6d482ad19897db33d334bffe4b294da11a36f 
  3rdparty/stout/include/stout/windows/os.hpp 
09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
  3rdparty/stout/tests/os_tests.cpp 959b02d08f06481a64ef1a39acf23b062d5dc805 


Diff: https://reviews.apache.org/r/63245/diff/2/

Changes: https://reviews.apache.org/r/63245/diff/1-2/


Testing
---

Modified tests:
TEST_F(OsTest, Libraries)
TEST_F(OsTest, Sleep)
TEST_F(OsTest, SYMLINK_Size)
TEST_F(OsTest, SYMLINK_Realpath)


Thanks,

Raluca Miclea

Re: Review Request 63245: Ported and enabled Os Tests on Windows.

[Raluca Miclea]

> On Oct. 24, 2017, 6:55 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/windows/stat.hpp
> > Line 112 (original), 112 (patched)
> > 
> >
> > Why was this change made?

The test failed. My guess was that substitute_name should have been used 
instead of print_name.


> On Oct. 24, 2017, 6:55 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp
> > Lines 390-392 (patched)
> > 
> >
> > This should be an `Error`, not a `WindowsError`, as the latter is 
> > reserved for OS errors (explicitly, its implementation calls 
> > `GetLastError()` and retrieves the Windows error message, none of which 
> > should happen here).
> > 
> > While the POSIX implementation does not explicitly check for `duration 
> > < 0`, since the system call itself fails, and that failure is returned, I 
> > agree that it's reasonable here to return an `Error`. However, a note 
> > should be added to the function as to why we do that (specificically, so 
> > that the `os::sleep` API remains consistent).

I'll fix it.


> On Oct. 24, 2017, 6:55 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/os_tests.cpp
> > Lines 213-215 (original), 213-215 (patched)
> > 
> >
> > If this is resolved, this comment needs to be removed.

Will do.


> On Oct. 24, 2017, 6:55 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/os_tests.cpp
> > Lines 232-233 (original)
> > 
> >
> > Removing this test code does not resolve this issue. In fact, it's 
> > reducing test coverage, as there is clearly a bug here highlighted by this 
> > failing test.
> > 
> > Instead, I agree with you here:
> > 
> > > If you are supposed to be able to test the size of a non-link file in
> > the context of not-following-a-link, the program should return the
> > size of the non-link file instead of returning an error upon 
> > determining the
> > nature of the path as not being a link file path.
> > 
> > So the correct solution is to fix `os::stat::size` on Windows to work 
> > correctly, at which point this test can be enabled.

I'll look into that.


> On Oct. 24, 2017, 6:55 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/os_tests.cpp
> > Lines 274-275 (original), 272-273 (patched)
> > 
> >
> > If this is working now, the TODO here should be removed.

Will do.


> On Oct. 24, 2017, 6:55 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/os_tests.cpp
> > Lines 885-925 (original), 883-923 (patched)
> > 
> >
> > Having changed `ldLibraryPath` above to `PATH`, this just potentially 
> > broke numerous things by overriding the system's `PATH` (hopefully just for 
> > this process, but I digress, it's incorrect).
> > 
> > This test sh ould probably _never_ be enabled on Windows, as there is 
> > simply no equivalent to `LD_LIBRARY_PATH`. Windows loads libraries from a 
> > static set of paths that cannot be changed.
> > 
> > Instead of enabling this test, it should be explicitly `#ifdef'd` out 
> > with an explanation for Windows that the API simply cannot be ported.

I undersatnd. I'll fix that.


> On Oct. 24, 2017, 6:55 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/os_tests.cpp
> > Line 1023 (original), 1021 (patched)
> > 
> >
> > This is incorrect. This test is explicitly asserting that the 
> > `testLink` resolved to the `testFile`. Changing the test is wrong.
> > 
> > Instead, there is a set of tasks to fix `realpath` on Windows, 
> > including MESOS-6735, MESOS-5881, and even MESOS-7604.
> > 
> > For now, we'll leave fixing `realpath` separate to fixing these other 
> > tests.

Ok.


- Raluca


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63245/#review189074
---


On Oct. 24, 2017, 9:30 a.m., Raluca Miclea wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63245/
> ---
> 
> (Updated Oct. 24, 2017, 9:30 a.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-3441
> https://issues.apache.org/jira/browse/MESOS-3441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> TEST_F(OsTest, Libraries)
> 

Re: Review Request 63245: Ported and enabled Os Tests on Windows.

[Raluca Miclea]

> On Oct. 24, 2017, 6:55 p.m., Andrew Schwartzmeyer wrote:
> > This review request should really be at least three separate patches. THere 
> > is work to be done to fix several of these tests, and right now they're all 
> > mixed together.

should I discard this commit and make separate ones for each issue?


- Raluca


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63245/#review189074
---


On Oct. 24, 2017, 9:30 a.m., Raluca Miclea wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63245/
> ---
> 
> (Updated Oct. 24, 2017, 9:30 a.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-3441
> https://issues.apache.org/jira/browse/MESOS-3441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> TEST_F(OsTest, Libraries)
> The test failed because no environmental variable was provided for
> Windows. I used "PATH" as environmental variable on Windows since
> there's no default variable where the linker should look into while
> linking dynamic libraries/shared libraries. (like LD_LIBRARY_PATH on 
> Unix/Linux)
> 
> TEST_F(OsTest, Sleep)
> The test hanged because of the last assertion:
> ASSERT_ERROR(os::sleep(Milliseconds(-10)));
> Apparently, it didn't handle a negative sleep duration. By simply
> returning an error value when the duration is negative the problem was
> solved.
> 
> TEST_F(OsTest, SYMLINK_Size)
> First issue was this assertion:
> EXPECT_SOME_EQ(size, 
> os::stat::size(file,FollowSymlink::DO_NOT_FOLLOW_SYMLINK));
> It fails everytime because  is not a symbolic link. When calling
> os:stat:size with argument  set to "DO_NOT_FOLLOW_SYMLINK", it firstly
> checks whether the path it receives is a symlink in order to get the symlink 
> data
> for that path. If it's not a symlink, it returns an error.
> If you are supposed to be able to test the size of a non-link file in
> the context of not-following-a-link, the program should return the
> size of the non-link file instead of returning an error upon determining the
> nature of the path as not being a link file path. Right now, the assertion
> is not relevant since you'll get an error if you supply it a non-link file
> path rather thana symlink.
> 
> TEST_F(OsTest, SYMLINK_Realpath)
> The issue was that the symlink resolved to itself but it was tested
> against the path of the target file. Once the path of the target file
> was replaced by the path of the symlink, the test passed.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os.hpp 
> 1a81db61faa55d7043d75a012fe1e66b49bf601c 
>   3rdparty/stout/include/stout/os/windows/stat.hpp 
> 0db6d482ad19897db33d334bffe4b294da11a36f 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
>   3rdparty/stout/tests/os_tests.cpp 959b02d08f06481a64ef1a39acf23b062d5dc805 
> 
> 
> Diff: https://reviews.apache.org/r/63245/diff/1/
> 
> 
> Testing
> ---
> 
> Modified tests:
> TEST_F(OsTest, Libraries)
> TEST_F(OsTest, Sleep)
> TEST_F(OsTest, SYMLINK_Size)
> TEST_F(OsTest, SYMLINK_Realpath)
> 
> 
> Thanks,
> 
> Raluca Miclea
> 
>

Review Request 63331: Added documentation for the `network/ports` isolator.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63331/
---

Review request for mesos, Jie Yu and Qian Zhang.


Repository: mesos


Description
---

Added documentation for the `network/ports` isolator to the Mesos
containerizer documentation. Updated the agent flags documentation with
the new isolator flags. Added changelog information to note the new
isolator, the new agent flags and new fields in the TaskStatus message.


Diffs
-

  docs/configuration/agent.md 5c1b469f28d4a7af0aebad5da65a00dd0e6c2a6c 
  docs/isolators/network-ports.md PRE-CREATION 
  docs/mesos-containerizer.md 9cb071273a5ce42784457ea01b363911fdb7773d 
  docs/upgrades.md 6370c06a497161317884f61f376308423b744fe1 


Diff: https://reviews.apache.org/r/63331/diff/1/


Testing
---

Manually checked rendered website.


Thanks,

James Peach

Re: Review Request 63239: Ported and enabled Flag Tests on Windows.

[Raluca Miclea]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63239/
---

(Updated Oct. 26, 2017, 11:01 a.m.)


Review request for mesos and Andrew Schwartzmeyer.


Changes
---

removed TODO comments


Bugs: MESOS-3437
https://issues.apache.org/jira/browse/MESOS-3437


Repository: mesos


Description
---

They worked out of the box.
Apparently the issues MESOS-5880 and MESOS-5937 which were causing the tests to 
fail were solved.


Diffs (updated)
-

  3rdparty/stout/tests/flags_tests.cpp 88c8ee57c2bf328251513761498ef98d774be006 


Diff: https://reviews.apache.org/r/63239/diff/2/

Changes: https://reviews.apache.org/r/63239/diff/1-2/


Testing
---

Modified Tests:
TEST(FlagsTest, LoadFromEnvironment)
TEST(FlagsTest, DuplicatesFromEnvironment)


Thanks,

Raluca Miclea

Re: Review Request 63238: Ported and enabled Path Tests on Windows.

[Raluca Miclea]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63238/
---

(Updated Oct. 26, 2017, 10:56 a.m.)


Review request for mesos and Andrew Schwartzmeyer.


Changes
---

addressed review part 2


Bugs: MESOS-3442
https://issues.apache.org/jira/browse/MESOS-3442


Repository: mesos


Description (updated)
---

Used path::join() to join them with the os:PATH_SEPARATOR, which is '\\'
on Windows and '/' elsewhere. When testing path::join() itself, I
constructed path strings using the os::PATH_SEPARATOR for portability.


Diffs (updated)
-

  3rdparty/stout/tests/path_tests.cpp f8c14d5aefe0b49adb778da784143a328c96183d 


Diff: https://reviews.apache.org/r/63238/diff/3/

Changes: https://reviews.apache.org/r/63238/diff/2-3/


Testing
---

Modified Tests:
TEST(PathTest, Basename)
TEST(PathTest, Dirname)
TEST(PathTest, Extension)
TEST(PathTest, Join)


Thanks,

Raluca Miclea

Re: Review Request 63238: Ported and enabled Path Tests on Windows.

[Raluca Miclea]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63238/
---

(Updated Oct. 26, 2017, 10:33 a.m.)


Review request for mesos and Andrew Schwartzmeyer.


Changes
---

addressed review


Bugs: MESOS-3442
https://issues.apache.org/jira/browse/MESOS-3442


Repository: mesos


Description
---

I checked #ifdefs in order to decide what tests to build and run on
Window and Posix systems respectively.
For Windows tests I mainly replaced the "slash" path separator with
"backslash".
I extracted common assertion before the #ifdef directive.


Diffs (updated)
-

  3rdparty/stout/tests/flags_tests.cpp 88c8ee57c2bf328251513761498ef98d774be006 
  3rdparty/stout/tests/path_tests.cpp f8c14d5aefe0b49adb778da784143a328c96183d 


Diff: https://reviews.apache.org/r/63238/diff/2/

Changes: https://reviews.apache.org/r/63238/diff/1-2/


Testing
---

Modified Tests:
TEST(PathTest, Basename)
TEST(PathTest, Dirname)
TEST(PathTest, Extension)
TEST(PathTest, Join)


Thanks,

Raluca Miclea

Re: Review Request 63293: Update google-glog to 0.3.5.

[Tomasz Janiszewski]

> On Oct. 26, 2017, 1:43 a.m., Andrew Schwartzmeyer wrote:
> > Is there a pressing need to update this _right now_? As it is, we are 
> > waiting for 0.3.6 (MESOS-3394) so that we can converge on a single version 
> > of Glog for all platforms.

Nope. We can wait for 0.3.6. We have merged config.guess that open a way to 
build on ARM. I'll discard this PR. I'm not sure if our all issues will be 
solved in 0.3.6. The glog patch contains fix for cmake build. I'm not sure if 
glog 0.3.6 fixes this issue. 
I'm going to close this PR.


- Tomasz


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63293/#review189264
---


On Oct. 25, 2017, 12:28 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63293/
> ---
> 
> (Updated Oct. 25, 2017, 12:28 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3322
> https://issues.apache.org/jira/browse/MESOS-3322
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Version 0.3.5 adds support for PowerPC so no patching is required.
> 
> Fixes: MESOS-3322
> Refs: https://github.com/google/glog/releases/tag/v0.3.5
> 
> 
> Diffs
> -
> 
>   3rdparty/glog-0.3.5.patch ab15bea04d7b8af31a638b7c4ce3256ed2f0df4e 
> 
> 
> Diff: https://reviews.apache.org/r/63293/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 61473: Do not kill non partition aware tasks.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61473/#review188966
---




src/master/http.cpp
Line 317 (original), 317 (patched)


This comment isn't about this line but it seems my previous comment at the 
top of my last review was overlooked so I am raising an issue here :)

For this patch we should also update the comments above PARTITION_AWARE API 
to reflect the change.

https://github.com/apache/mesos/blob/6eefc685ccf304d0fb8ed4ff9bc314197d77f078/include/mesos/mesos.proto#L336

Also please search the docs for necessary changes.



src/master/http.cpp
Lines 322-324 (patched)


Seems like if the framework is NPA, we don't have to iterate through the 
list? i.e., put the check outside the `foreachvalue` loop?



src/master/http.cpp
Lines 345-346 (patched)


Given that we now store all NPA tasks in TASK_LOST (instead of being 
converted to TASK_LOST on the fly here), is it more straightfoward to say 

```
// Unreachable tasks belonging to a non-partition-aware framework have been 
stored as TASK_LOST for backward compatibility so we should export them as 
completed tasks.
```

?



src/master/http.cpp
Lines 348-350 (patched)


The reason we are here is that tasks in `framework_->unreachableTasks` 
could be in `TASK_LOST` state and we should export those as "completed_tasks".

So I think the condition check isn't right? `if 
(framework_->capabilities.partitionAware)` is going to skip all tasks 
previously stored as `TASK_LOST`.

Like I suggested in another place, should we be checking against `if 
(task.get()->state() != TASK_LOST)` directly?



src/master/http.cpp
Lines 4225 (patched)


Is is more straightfoward to do `if (task.get()->state() == 
TASK_UNREACHABLE)` than the inequality check?



src/master/http.cpp
Lines 4228-4229 (patched)


Ditto on the update of the comment.



src/master/master.hpp
Lines 2591 (patched)


You made a redundant copy here but I understand this line may go away 
anyways. :)



src/master/master.hpp
Line 2588 (original), 2594 (patched)


Sorry we have gone back and forth but for now let's use this API:

```
void removeTask(Task* task, bool unreachable)
```

I originally hoped to solely rely on `task->state()` in the workflow but it 
turned out to be pretty difficult. So here let's add a comment above the method 
explaning the reason for the boolean.

e.g.,

```
// Removes the task. `unreachable` indicates whether the task is removed 
due to being unreachable. Note that we cannot rely on the task state because it 
may not reflect unreachability due to being set to TASK_LOST for backwards 
compatibility.
```



src/master/master.cpp
Lines 9683-9684 (original), 9637-9639 (patched)


If the framework is PA, it can still contain tasks in `TASK_LOST`. For now 
let's follow the implementation in `Master::_tasks_killing()`: check the state 
of each task.



src/tests/partition_tests.cpp
Lines 2044-2045 (patched)


A comment shouldn't split the chain. I think 

```
.WillRepeatedly(Return()); // The agent may resend status updates.
```

fits in one line?


- Jiang Yan Xu


On Oct. 16, 2017, 1:59 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61473/
> ---
> 
> (Updated Oct. 16, 2017, 1:59 a.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7215
> https://issues.apache.org/jira/browse/MESOS-7215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master will not kill the tasks for non-Partition aware frameworks
> when an unreachable agent re-registers with the master.
> Master used to send a ShutdownFrameworkMessages to the agent
> to kill the tasks from non partition aware frameworks including the
> ones that are still registered which was problematic because the offer
> from this agent could still go to the same framework which could then
> launch new tasks. The agent would then receive tasks of the same
> framework and ignore them because it thinks the framework is shutting
> down. The framework is 

Re: Review Request 63314: Add support for Ubuntu 16.04 in docker build.

[Tomasz Janiszewski]

> On Oct. 26, 2017, 7:55 a.m., Vinod Kone wrote:
> > support/docker-build.sh
> > Lines 72 (patched)
> > 
> >
> > do we need to install `zlib1g-dev` explicitly in ubuntu 16.04 but not 
> > in other versions?

I'm not sure if we need it in 14.04. I follow instructions from [getting 
started](http://mesos.apache.org/documentation/latest/building/#ubuntu-14-04) 
and it looks like this are the only difference between 14.04 and 16.04 
https://www.diffchecker.com/Aj4N9M8y


- Tomasz


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63314/#review189278
---


On Oct. 26, 2017, 8:08 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63314/
> ---
> 
> (Updated Oct. 26, 2017, 8:08 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for Ubuntu 16.04 in docker build.
> 
> 
> Diffs
> -
> 
>   support/docker-build.sh 50e693ca71d499f71f2935923f4691feef560b12 
> 
> 
> Diff: https://reviews.apache.org/r/63314/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 63314: Add support for Ubuntu 16.04 in docker build.

[Tomasz Janiszewski]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63314/
---

(Updated Oct. 26, 2017, 8:08 a.m.)


Review request for mesos, Jie Yu and Vinod Kone.


Changes
---

Make `apt-get install -y` consistent


Repository: mesos


Description
---

Add support for Ubuntu 16.04 in docker build.


Diffs (updated)
-

  support/docker-build.sh 50e693ca71d499f71f2935923f4691feef560b12 


Diff: https://reviews.apache.org/r/63314/diff/2/

Changes: https://reviews.apache.org/r/63314/diff/1-2/


Testing
---


Thanks,

Tomasz Janiszewski

Re: Review Request 63315: Allowe override of parallel jobs in docker build.

[Vinod Kone]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63315/#review189279
---


Ship it!




Ship It!

- Vinod Kone


On Oct. 25, 2017, 9:58 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63315/
> ---
> 
> (Updated Oct. 25, 2017, 9:58 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use JOBS env variable to control `make -j` parameter.
> By default `JOBS=6`.
> 
> 
> Diffs
> -
> 
>   support/docker-build.sh 50e693ca71d499f71f2935923f4691feef560b12 
> 
> 
> Diff: https://reviews.apache.org/r/63315/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 63322: Added placeholder handlers and other changes for operation updates.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63322/#review189277
---


Fix it, then Ship it!





src/master/master.hpp
Lines 1005 (patched)


Ditto on naming. `acknowledgeOfferOperationStatusUpdate`



src/slave/slave.hpp
Lines 285 (patched)


`offerOperationStatusUpdateAcknowledgement`


- Jie Yu


On Oct. 26, 2017, 7:01 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63322/
> ---
> 
> (Updated Oct. 26, 2017, 7:01 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-8130
> https://issues.apache.org/jira/browse/MESOS-8130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds empty placeholder handler functions which will
> be used for offer operation status updates as well as their
> acknowledgement and reconciliation.
> 
> A number of switch statements are also updated to handle new
> enum values and validation code is added.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp f5d4bc5da26f232a1fad1169b0c656b19132b853 
>   src/examples/long_lived_framework.cpp 
> adfe10eca2af26e8294171256e5f053fb46abf24 
>   src/examples/test_http_framework.cpp 
> e640800055bd9a946663de377970e867d29c750a 
>   src/master/http.cpp 42139bec519d36316e324ef921157c49cdf2d043 
>   src/master/master.hpp a3c9530e340b14b739da90851227f8eed254b4ac 
>   src/master/master.cpp 4b76648b4887724e166ff95e002ee26c8bfef006 
>   src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb 
>   src/sched/sched.cpp fb77b79a05d5db21c8642cb8915e9bdcc1e2b1de 
>   src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef 
>   src/slave/slave.cpp d8477b4e364270f5ad61311aa8fe48f823bc7aac 
>   src/tests/mesos.hpp 4b61f2dbd67a5e8a65a6655519c8f03e5d6c6954 
> 
> 
> Diff: https://reviews.apache.org/r/63322/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 63314: Add support for Ubuntu 16.04 in docker build.

[Vinod Kone]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63314/#review189278
---




support/docker-build.sh
Lines 65-67 (original), 65-67 (patched)


not yours but can you change

s/-y install/install -y/ for consistency?



support/docker-build.sh
Lines 72 (patched)


do we need to install `zlib1g-dev` explicitly in ubuntu 16.04 but not in 
other versions?


- Vinod Kone


On Oct. 25, 2017, 9:41 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63314/
> ---
> 
> (Updated Oct. 25, 2017, 9:41 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for Ubuntu 16.04 in docker build.
> 
> 
> Diffs
> -
> 
>   support/docker-build.sh 50e693ca71d499f71f2935923f4691feef560b12 
> 
> 
> Diff: https://reviews.apache.org/r/63314/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 63321: Added protobuf messages for V1 scheduler operation feedback.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63321/#review189276
---




include/mesos/scheduler/scheduler.proto
Lines 386 (patched)


I'd prefer we use `OfferOperation` consistently. We might introduce some 
other operation in the future, just in case.


- Jie Yu


On Oct. 26, 2017, 6:58 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63321/
> ---
> 
> (Updated Oct. 26, 2017, 6:58 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-8131
> https://issues.apache.org/jira/browse/MESOS-8131
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds new and updated protobuf messages to facilitate
> offer operation status updates, as well as acknowledgement of
> those updates and operation status reconciliation.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> f82f74d1c3767f97a1e6dad8acf4602f39e18380 
>   include/mesos/v1/scheduler/scheduler.proto 
> 1fb0254103e5bb4f2cb98ca973e9f4e7b378 
>   src/messages/messages.proto 0a32b3457e9143a7d48670610ca3e56dd516136f 
> 
> 
> Diff: https://reviews.apache.org/r/63321/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 63321: Added protobuf messages for V1 scheduler operation feedback.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63321/#review189275
---


Ship it!




Ship It!

- Jie Yu


On Oct. 26, 2017, 6:58 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63321/
> ---
> 
> (Updated Oct. 26, 2017, 6:58 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-8131
> https://issues.apache.org/jira/browse/MESOS-8131
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds new and updated protobuf messages to facilitate
> offer operation status updates, as well as acknowledgement of
> those updates and operation status reconciliation.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> f82f74d1c3767f97a1e6dad8acf4602f39e18380 
>   include/mesos/v1/scheduler/scheduler.proto 
> 1fb0254103e5bb4f2cb98ca973e9f4e7b378 
>   src/messages/messages.proto 0a32b3457e9143a7d48670610ca3e56dd516136f 
> 
> 
> Diff: https://reviews.apache.org/r/63321/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 63322: Added placeholder handlers and other changes for operation updates.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63322/
---

Review request for mesos, Gaston Kleiman, Jie Yu, and Vinod Kone.


Bugs: MESOS-8130
https://issues.apache.org/jira/browse/MESOS-8130


Repository: mesos


Description
---

This patch adds empty placeholder handler functions which will
be used for offer operation status updates as well as their
acknowledgement and reconciliation.

A number of switch statements are also updated to handle new
enum values and validation code is added.


Diffs
-

  src/cli/execute.cpp f5d4bc5da26f232a1fad1169b0c656b19132b853 
  src/examples/long_lived_framework.cpp 
adfe10eca2af26e8294171256e5f053fb46abf24 
  src/examples/test_http_framework.cpp e640800055bd9a946663de377970e867d29c750a 
  src/master/http.cpp 42139bec519d36316e324ef921157c49cdf2d043 
  src/master/master.hpp a3c9530e340b14b739da90851227f8eed254b4ac 
  src/master/master.cpp 4b76648b4887724e166ff95e002ee26c8bfef006 
  src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb 
  src/sched/sched.cpp fb77b79a05d5db21c8642cb8915e9bdcc1e2b1de 
  src/slave/slave.hpp aea1e948209c7c8945665915bc2f6d8eb47814ef 
  src/slave/slave.cpp d8477b4e364270f5ad61311aa8fe48f823bc7aac 
  src/tests/mesos.hpp 4b61f2dbd67a5e8a65a6655519c8f03e5d6c6954 


Diff: https://reviews.apache.org/r/63322/diff/1/


Testing
---

`make check`


Thanks,

Greg Mann

Review Request 63321: Added protobuf messages for V1 scheduler operation feedback.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63321/
---

Review request for mesos, Gaston Kleiman, Jie Yu, and Vinod Kone.


Bugs: MESOS-8131
https://issues.apache.org/jira/browse/MESOS-8131


Repository: mesos


Description
---

This patch adds new and updated protobuf messages to facilitate
offer operation status updates, as well as acknowledgement of
those updates and operation status reconciliation.


Diffs
-

  include/mesos/scheduler/scheduler.proto 
f82f74d1c3767f97a1e6dad8acf4602f39e18380 
  include/mesos/v1/scheduler/scheduler.proto 
1fb0254103e5bb4f2cb98ca973e9f4e7b378 
  src/messages/messages.proto 0a32b3457e9143a7d48670610ca3e56dd516136f 


Diff: https://reviews.apache.org/r/63321/diff/1/


Testing
---

Testing details can be found at the end of this chain.


Thanks,

Greg Mann