[GitHub] [mesos] akornatskyy commented on a change in pull request #361: Added ability to specify a root dir for boost-libs and curl.

[GitBox]

akornatskyy commented on a change in pull request #361:
URL: https://github.com/apache/mesos/pull/361#discussion_r416710336



##
File path: 3rdparty/CMakeLists.txt
##
@@ -696,7 +701,16 @@ if (ENABLE_SECCOMP_ISOLATOR)
   URL   ${LIBSECCOMP_URL}
   URL_HASH  ${LIBSECCOMP_HASH})
   else ()
-find_package(LIBSECCOMP REQUIRED)
+if ("${LIBSECCOMP_ROOT_DIR}" STREQUAL "")
+  find_package(LIBSECCOMP REQUIRED)
+else ()
+  set(POSSIBLE_LIBSECCOMP_INCLUDE_DIRS ${LIBSECCOMP_ROOT_DIR}/include)
+  set(POSSIBLE_LIBSECCOMP_LIB_DIRS ${LIBSECCOMP_ROOT_DIR}/lib)
+
+  set(LIBSECCOMP_LIBRARY_NAMES seccomp)
+
+  FIND_PACKAGE_HELPER(LIBSECCOMP seccomp.h)
+endif ()

Review comment:
   yes, done. checked.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: Review Request 72445: Fixed operator api event inconsistencies issue.

[Dong Zhu]

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

(Updated April 28, 2020, 9:13 p.m.)


Review request for mesos and Andrei Sekretenko.


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


Repository: mesos


Description
---

This patch intends to fix issue MESOS-10085.

When the authorization failed happens master return nothing to the
subscriber, subscriber isn't aware of what is happening, this issue
can lead to inconsistencies in Event stream.

Signed-off-by: Dong Zhu 


Diffs
-

  include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 
  include/mesos/v1/master/master.proto 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d 
  src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c 


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


Testing
---


Thanks,

Dong Zhu

[GitHub] [mesos] greggomann commented on a change in pull request #361: Added ability to specify a root dir for boost-libs, curl and libseccomp.

[GitBox]

greggomann commented on a change in pull request #361:
URL: https://github.com/apache/mesos/pull/361#discussion_r416643733



##
File path: 3rdparty/CMakeLists.txt
##
@@ -696,7 +701,16 @@ if (ENABLE_SECCOMP_ISOLATOR)
   URL   ${LIBSECCOMP_URL}
   URL_HASH  ${LIBSECCOMP_HASH})
   else ()
-find_package(LIBSECCOMP REQUIRED)
+if ("${LIBSECCOMP_ROOT_DIR}" STREQUAL "")
+  find_package(LIBSECCOMP REQUIRED)
+else ()
+  set(POSSIBLE_LIBSECCOMP_INCLUDE_DIRS ${LIBSECCOMP_ROOT_DIR}/include)
+  set(POSSIBLE_LIBSECCOMP_LIB_DIRS ${LIBSECCOMP_ROOT_DIR}/lib)
+
+  set(LIBSECCOMP_LIBRARY_NAMES seccomp)
+
+  FIND_PACKAGE_HELPER(LIBSECCOMP seccomp.h)
+endif ()

Review comment:
   Check if we can get rid of this?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: Review Request 72399: Updated UCR's `usage()` method to support resource limits.

[Greg Mann]

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




src/slave/containerizer/mesos/containerizer.cpp
Lines 2460-2463 (original), 2459-2465 (patched)


Instead of having both the `containerConfig` argument as well as 
`resourceRequests`/`resourceLimits`, couldn't we get rid of `containerConfig` 
and use a conditional check for `if (containerId.has_parent())` in `usage()` to 
control whether we get the request/limit values from the `ContainerConfig` or 
from the `Container` struct?



src/slave/containerizer/mesos/containerizer.cpp
Lines 2481-2486 (original), 2483-2508 (patched)


Can we move this logic into the conditionals for `if (cpuLimit.isSome())` 
and `if (memLimit.isSome())` below?



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


Nit: make `resources` a const ref?



src/slave/containerizer/mesos/containerizer.cpp
Lines 2533-2538 (patched)


Do you want to move this `foreach` loop out of the conditional, since it's 
shared by both branches?



src/slave/containerizer/mesos/containerizer.cpp
Lines 2517-2519 (original), 2589-2594 (patched)


This stuff looks stale to me, do you think we can remove it?


- Greg Mann


On April 25, 2020, 10:55 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72399/
> ---
> 
> (Updated April 25, 2020, 10:55 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10117
> https://issues.apache.org/jira/browse/MESOS-10117
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated UCR's `usage()` method to support resource limits.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 2ea033aa1869f07644b10c036fda1698d08aa89b 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 6aa4f3fe0940575aeea6a63cdb1ca3c77cd2359b 
>   src/tests/slave_recovery_tests.cpp 0efd3a6ac09ad06d9365b7bb2295157b5175e6b8 
>   src/tests/slave_tests.cpp 6b264d067accb9800ba75ea8f5d27c1e4c5593db 
> 
> 
> Diff: https://reviews.apache.org/r/72399/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72398: Added `cpus_soft_limit` field to `ResourceStatistics` protobuf message.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On April 24, 2020, 12:24 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72398/
> ---
> 
> (Updated April 24, 2020, 12:24 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10117
> https://issues.apache.org/jira/browse/MESOS-10117
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `cpus_soft_limit` field to `ResourceStatistics` protobuf message.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 9412ed736231547b22abc89188316b08d5445e78 
>   include/mesos/v1/mesos.proto 194c42cf57e34d803a21cab03db17614855e8692 
> 
> 
> Diff: https://reviews.apache.org/r/72398/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72401: Updated Docker containerizer by not updating resources for command task.

[Greg Mann]

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



Could you update the description of the patch to describe the difference 
between the two cases of a command task and a task with a Docker executor 
container?


src/slave/containerizer/docker.hpp
Lines 357 (patched)


Nit: you could use the `? :` ternary operator to initialize 
`generatedForCommandTask` here in the initializer list, I don't think it 
matters too much but it might be more readable if somebody just glances at the 
initializer list to see what the value is initialized to.


- Greg Mann


On April 22, 2020, 8:33 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72401/
> ---
> 
> (Updated April 22, 2020, 8:33 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-8877
> https://issues.apache.org/jira/browse/MESOS-8877
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Docker containerizer by not updating resources for command task.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 09fc2795289e1292134b7d4bb2cc079c80d91c3d 
>   src/slave/containerizer/docker.cpp 492ac273fafb55d3e4c90d70fbf9c8c0c4fe7e70 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> b069f518d51225c39f0cd5126c02fa08674566ab 
> 
> 
> Diff: https://reviews.apache.org/r/72401/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72402: Updated Docker containerizer's `usage()` to support resource limits.

[Greg Mann]

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




src/slave/containerizer/docker.cpp
Lines 2077-2099 (patched)


What do you think about moving this logic into the conditionals below where 
`totalCPUs` and `totalMem` are used?



src/slave/containerizer/docker.cpp
Lines 2164-2167 (patched)


Should we have an `#else` case here which does not check the value of 
`cgroups_enable_cfs`?



src/slave/containerizer/docker.cpp
Line 2095 (original), 2179-2181 (patched)


Why don't we set the soft limit in this case?


- Greg Mann


On April 25, 2020, 10:55 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72402/
> ---
> 
> (Updated April 25, 2020, 10:55 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10117
> https://issues.apache.org/jira/browse/MESOS-10117
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Docker containerizer's `usage()` to support resource limits.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 492ac273fafb55d3e4c90d70fbf9c8c0c4fe7e70 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> b069f518d51225c39f0cd5126c02fa08674566ab 
> 
> 
> Diff: https://reviews.apache.org/r/72402/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72448: Fixed operator api event inconsistencies issue.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On April 29, 2020, 3:30 a.m., Dong Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72448/
> ---
> 
> (Updated April 29, 2020, 3:30 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch intends to fix issue MESOS-10085.
> 
> When the authorization failed happens master return nothing to the
> subscriber, subscriber isn't aware of what is happening, this issue
> can lead to inconsistencies in Event stream.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 
>   include/mesos/v1/master/master.proto 
> 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d 
>   src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c 
>   src/tests/master/mock_master_api_subscriber.cpp 
> 893d3e366164ccebd2847ed4c2874ab00e0e5b7b 
> 
> 
> Diff: https://reviews.apache.org/r/72448/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dong Zhu
> 
>

[GitHub] [mesos] akornatskyy commented on a change in pull request #361: Added ability to specify a root dir for boost-libs and curl.

[GitBox]

akornatskyy commented on a change in pull request #361:
URL: https://github.com/apache/mesos/pull/361#discussion_r417074587



##
File path: 3rdparty/CMakeLists.txt
##
@@ -194,29 +194,34 @@ endfunction()
 # Boost: C++ Libraries.
 # http://www.boost.org
 ###
-EXTERNAL(boost ${BOOST_VERSION} ${CMAKE_CURRENT_BINARY_DIR})
-add_library(boost INTERFACE)
-add_dependencies(boost ${BOOST_TARGET})
-if (CMAKE_CXX_COMPILER_ID MATCHES GNU OR CMAKE_CXX_COMPILER_ID MATCHES Clang)
-  # Headers including Boost 1.65.0 fail to compile with GCC 7.2 and
-  # CLang 3.6 without `-Wno-unused-local-typedefs`.
-  # TODO(andschwa): Remove this when Boost has a resolution.
-  target_compile_options(boost INTERFACE -Wno-unused-local-typedefs)
-endif ()
-target_include_directories(boost INTERFACE ${BOOST_ROOT})
+if ("${BOOST_ROOT_DIR}" STREQUAL "")
+  EXTERNAL(boost ${BOOST_VERSION} ${CMAKE_CURRENT_BINARY_DIR})
+  add_library(boost INTERFACE)
+  add_dependencies(boost ${BOOST_TARGET})
+  if (CMAKE_CXX_COMPILER_ID MATCHES GNU OR CMAKE_CXX_COMPILER_ID MATCHES Clang)
+# Headers including Boost 1.65.0 fail to compile with GCC 7.2 and
+# CLang 3.6 without `-Wno-unused-local-typedefs`.
+# TODO(andschwa): Remove this when Boost has a resolution.
+target_compile_options(boost INTERFACE -Wno-unused-local-typedefs)
+  endif ()
+  target_include_directories(boost INTERFACE ${BOOST_ROOT})

Review comment:
   sounds reasonable to me, will give it a try.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: Review Request 72410: Updated existing documentation for resource limits.

[Greg Mann]

> On April 28, 2020, 9:59 a.m., Qian Zhang wrote:
> > docs/task-state-reasons.md
> > Lines 126-133 (patched)
> > 
> >
> > If we revert the changes about 
> > `REASON_CONTAINER_MEMORY_REQUEST_EXCEEDED` in 
> > https://reviews.apache.org/r/72442 , then we do not need to update this doc.

I'm not yet convinced that we should remove the new reason, I'd like to better 
understand why it's not being sent properly and hopefully fix it.


> On April 28, 2020, 9:59 a.m., Qian Zhang wrote:
> > docs/upgrades.md
> > Lines 52 (patched)
> > 
> >
> > I think we'd better to have a general description about the task 
> > resource limits feature (like how [quota 
> > limits](https://github.com/apache/mesos/blob/master/docs/upgrades.md#1-9-x-quota-guarantees)
> >  feature was described) rather than just mention CPU limits here.

I think this document isn't intended to be a changelog, but rather a guide for 
operators who need to upgrade a cluster. So, I think we should only mention 
things here which an operator might run into when upgrading a cluster: changes 
in existing behavior, possible issues that could occur, etc.

Regarding the quota example, IMHO the passage on quota limits could be reduced 
to the following text:

```
* Quota guarantees, while still functional in Mesos 1.9, are now deprecated. In 
the future when optimistic offers are fully implemented, a combination of the 
newly-added quota limits and priority-based preemption will be simpler to use.
```


- Greg


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


On April 28, 2020, 12:59 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72410/
> ---
> 
> (Updated April 28, 2020, 12:59 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Benjamin Mahler, Qian Zhang, and Tim 
> Harper.
> 
> 
> Bugs: MESOS-10115
> https://issues.apache.org/jira/browse/MESOS-10115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated existing documentation for resource limits.
> 
> 
> Diffs
> -
> 
>   docs/nested-container-and-task-group.md 
> 3325ff8e3a694ede82e22b24ab258742746c0bb7 
>   docs/scheduler-http-api.md 9831d527cc1f832a6fb0d0d330ebdc2a0b0f3774 
>   docs/task-state-reasons.md 10cb7e08d854ecfd16152fd370e4efe03b327347 
>   docs/upgrades.md 1e73e3d2626b7de93f5907bb52e008830f55e3a0 
> 
> 
> Diff: https://reviews.apache.org/r/72410/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72409: Added intro docs on running workloads.

[Greg Mann]

> On April 22, 2020, 2:09 p.m., Qian Zhang wrote:
> > docs/running-workloads.md
> > Lines 65-67 (patched)
> > 
> >
> > Do we want to mention how we set OOM score adjustment for burstable 
> > tasks somewhere in the doc?
> > 
> > And I think we also need to describe the newly introduced 
> > `REASON_CONTAINER_MEMORY_REQUEST_EXCEEDED` in the doc 
> > `task-state-reasons.md`.
> 
> Greg Mann wrote:
> I added these to https://reviews.apache.org/r/72410/ as well
> 
> Qian Zhang wrote:
> Yeah, I see you added how we set OOM score adjustment in 
> `nested-container-and-task-group.md` which is specific for nested container, 
> but we also set OOM score adjustment for burstable command tasks.

I added a clarification in the command task section saying that fields in the 
task message do the same thing in that case, that seems sufficient to me?


- Greg


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


On April 29, 2020, 12:01 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72409/
> ---
> 
> (Updated April 29, 2020, 12:01 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Benjamin Mahler, Qian Zhang, and Tim 
> Harper.
> 
> 
> Bugs: MESOS-10115
> https://issues.apache.org/jira/browse/MESOS-10115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a new document containing an introduction for
> scheduler authors to the available methods for running
> workloads in a Mesos cluster.
> 
> 
> Diffs
> -
> 
>   docs/home.md 027319fa28abbdac21588ad1c62dae588033f60e 
>   docs/running-workloads.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72409/diff/3/
> 
> 
> Testing
> ---
> 
> Viewed with `site/mesos-website-dev.sh`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72410: Updated existing documentation for resource limits.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [72408, 72409, 72410]

Passed command: export OS='ubuntu:16.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/jenkins/buildbot.sh

- Mesos Reviewbot


On April 28, 2020, 12:59 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72410/
> ---
> 
> (Updated April 28, 2020, 12:59 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Benjamin Mahler, Qian Zhang, and Tim 
> Harper.
> 
> 
> Bugs: MESOS-10115
> https://issues.apache.org/jira/browse/MESOS-10115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated existing documentation for resource limits.
> 
> 
> Diffs
> -
> 
>   docs/nested-container-and-task-group.md 
> 3325ff8e3a694ede82e22b24ab258742746c0bb7 
>   docs/scheduler-http-api.md 9831d527cc1f832a6fb0d0d330ebdc2a0b0f3774 
>   docs/task-state-reasons.md 10cb7e08d854ecfd16152fd370e4efe03b327347 
>   docs/upgrades.md 1e73e3d2626b7de93f5907bb52e008830f55e3a0 
> 
> 
> Diff: https://reviews.apache.org/r/72410/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72413: Updated executor API docs to include the domain socket.

[Greg Mann]

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

(Updated April 28, 2020, 11:08 p.m.)


Review request for mesos, Benjamin Mahler and Vinod Kone.


Repository: mesos


Description
---

Updated executor API docs to include the domain socket.


Diffs (updated)
-

  docs/executor-http-api.md 4af4cd444dbeb29c115c46444dc01da0e86fd848 


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

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


Testing
---

Viewed with `site/mesos-website-dev.sh`.


Thanks,

Greg Mann

Re: Review Request 72409: Added intro docs on running workloads.

[Greg Mann]

> On April 22, 2020, 1:21 p.m., Qian Zhang wrote:
> > docs/running-workloads.md
> > Lines 30 (patched)
> > 
> >
> > What do you mean for "task is specified with executor ID"? Why do we 
> > want to specify an executor ID for a task?
> 
> Greg Mann wrote:
> In the LAUNCH_GROUP call, one or more tasks is specified along with an 
> executor info, so there is always an executor ID specified.
> 
> Greg Mann wrote:
> I updated the text to hopefully make this a little clearer, let me know 
> what you think.
> 
> Qian Zhang wrote:
> I see the updated text is:
> > When one task is specified with an executor that has a unique executor 
> ID
> 
> But IIUC, for the `LAUNCH_GROUP` call, the executor is not specified to 
> the individual task (i.e., `TaskInfo.executor`) in the task group, instead it 
> is specified in the `LaunchGroup` protobuf message, right?
> ```
> message LaunchGroup {
>   required ExecutorInfo executor = 1;
>   required TaskGroupInfo task_group = 2;
> }
> ```
> 
> Greg Mann wrote:
> Yes, I think the language "one task is specified with an executor" makes 
> sense in that context.

Hm, maybe I'll change it to "one task is specified alongside an executor" to 
make the distinction clearer?


- Greg


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


On April 27, 2020, 11:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72409/
> ---
> 
> (Updated April 27, 2020, 11:57 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Benjamin Mahler, Qian Zhang, and Tim 
> Harper.
> 
> 
> Bugs: MESOS-10115
> https://issues.apache.org/jira/browse/MESOS-10115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a new document containing an introduction for
> scheduler authors to the available methods for running
> workloads in a Mesos cluster.
> 
> 
> Diffs
> -
> 
>   docs/home.md 027319fa28abbdac21588ad1c62dae588033f60e 
>   docs/running-workloads.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72409/diff/2/
> 
> 
> Testing
> ---
> 
> Viewed with `site/mesos-website-dev.sh`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

[GitHub] [mesos] greggomann commented on a change in pull request #361: Added ability to specify a root dir for boost-libs and curl.

[GitBox]

greggomann commented on a change in pull request #361:
URL: https://github.com/apache/mesos/pull/361#discussion_r416997093



##
File path: 3rdparty/CMakeLists.txt
##
@@ -194,29 +194,34 @@ endfunction()
 # Boost: C++ Libraries.
 # http://www.boost.org
 ###
-EXTERNAL(boost ${BOOST_VERSION} ${CMAKE_CURRENT_BINARY_DIR})
-add_library(boost INTERFACE)
-add_dependencies(boost ${BOOST_TARGET})
-if (CMAKE_CXX_COMPILER_ID MATCHES GNU OR CMAKE_CXX_COMPILER_ID MATCHES Clang)
-  # Headers including Boost 1.65.0 fail to compile with GCC 7.2 and
-  # CLang 3.6 without `-Wno-unused-local-typedefs`.
-  # TODO(andschwa): Remove this when Boost has a resolution.
-  target_compile_options(boost INTERFACE -Wno-unused-local-typedefs)
-endif ()
-target_include_directories(boost INTERFACE ${BOOST_ROOT})
+if ("${BOOST_ROOT_DIR}" STREQUAL "")
+  EXTERNAL(boost ${BOOST_VERSION} ${CMAKE_CURRENT_BINARY_DIR})
+  add_library(boost INTERFACE)
+  add_dependencies(boost ${BOOST_TARGET})
+  if (CMAKE_CXX_COMPILER_ID MATCHES GNU OR CMAKE_CXX_COMPILER_ID MATCHES Clang)
+# Headers including Boost 1.65.0 fail to compile with GCC 7.2 and
+# CLang 3.6 without `-Wno-unused-local-typedefs`.
+# TODO(andschwa): Remove this when Boost has a resolution.
+target_compile_options(boost INTERFACE -Wno-unused-local-typedefs)
+  endif ()
+  target_include_directories(boost INTERFACE ${BOOST_ROOT})

Review comment:
   Hmm actually I'm wondering should we do the same thing we do for 
curl and boost that we do for seccomp, and have a `.cmake` file which defines 
helpers for those libraries as well?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: Review Request 72409: Added intro docs on running workloads.

[Greg Mann]

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

(Updated April 29, 2020, 12:01 a.m.)


Review request for mesos, Andrei Budnik, Benjamin Mahler, Qian Zhang, and Tim 
Harper.


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


Repository: mesos


Description
---

This adds a new document containing an introduction for
scheduler authors to the available methods for running
workloads in a Mesos cluster.


Diffs (updated)
-

  docs/home.md 027319fa28abbdac21588ad1c62dae588033f60e 
  docs/running-workloads.md PRE-CREATION 


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

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


Testing
---

Viewed with `site/mesos-website-dev.sh`


Thanks,

Greg Mann

Review Request 72448: Fixed operator api event inconsistencies issue.

[Dong Zhu]

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

Review request for mesos.


Repository: mesos


Description
---

This patch intends to fix issue MESOS-10085.

When the authorization failed happens master return nothing to the
subscriber, subscriber isn't aware of what is happening, this issue
can lead to inconsistencies in Event stream.


Diffs
-

  include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 
  include/mesos/v1/master/master.proto 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d 
  src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c 
  src/tests/master/mock_master_api_subscriber.cpp 
893d3e366164ccebd2847ed4c2874ab00e0e5b7b 


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


Testing
---


Thanks,

Dong Zhu

Re: Review Request 72409: Added intro docs on running workloads.

[Greg Mann]

> On April 22, 2020, 1:21 p.m., Qian Zhang wrote:
> > docs/running-workloads.md
> > Lines 30 (patched)
> > 
> >
> > What do you mean for "task is specified with executor ID"? Why do we 
> > want to specify an executor ID for a task?
> 
> Greg Mann wrote:
> In the LAUNCH_GROUP call, one or more tasks is specified along with an 
> executor info, so there is always an executor ID specified.
> 
> Greg Mann wrote:
> I updated the text to hopefully make this a little clearer, let me know 
> what you think.
> 
> Qian Zhang wrote:
> I see the updated text is:
> > When one task is specified with an executor that has a unique executor 
> ID
> 
> But IIUC, for the `LAUNCH_GROUP` call, the executor is not specified to 
> the individual task (i.e., `TaskInfo.executor`) in the task group, instead it 
> is specified in the `LaunchGroup` protobuf message, right?
> ```
> message LaunchGroup {
>   required ExecutorInfo executor = 1;
>   required TaskGroupInfo task_group = 2;
> }
> ```

Yes, I think the language "one task is specified with an executor" makes sense 
in that context.


- Greg


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


On April 27, 2020, 11:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72409/
> ---
> 
> (Updated April 27, 2020, 11:57 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Benjamin Mahler, Qian Zhang, and Tim 
> Harper.
> 
> 
> Bugs: MESOS-10115
> https://issues.apache.org/jira/browse/MESOS-10115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a new document containing an introduction for
> scheduler authors to the available methods for running
> workloads in a Mesos cluster.
> 
> 
> Diffs
> -
> 
>   docs/home.md 027319fa28abbdac21588ad1c62dae588033f60e 
>   docs/running-workloads.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72409/diff/2/
> 
> 
> Testing
> ---
> 
> Viewed with `site/mesos-website-dev.sh`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72409: Added intro docs on running workloads.

[Qian Zhang]

> On April 22, 2020, 9:21 p.m., Qian Zhang wrote:
> > docs/running-workloads.md
> > Lines 30 (patched)
> > 
> >
> > What do you mean for "task is specified with executor ID"? Why do we 
> > want to specify an executor ID for a task?
> 
> Greg Mann wrote:
> In the LAUNCH_GROUP call, one or more tasks is specified along with an 
> executor info, so there is always an executor ID specified.
> 
> Greg Mann wrote:
> I updated the text to hopefully make this a little clearer, let me know 
> what you think.

I see the updated text is:
> When one task is specified with an executor that has a unique executor ID

But IIUC, for the `LAUNCH_GROUP` call, the executor is not specified to the 
individual task (i.e., `TaskInfo.executor`) in the task group, instead it is 
specified in the `LaunchGroup` protobuf message, right?
```
message LaunchGroup {
  required ExecutorInfo executor = 1;
  required TaskGroupInfo task_group = 2;
}
```


- Qian


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


On April 28, 2020, 7:57 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72409/
> ---
> 
> (Updated April 28, 2020, 7:57 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Benjamin Mahler, Qian Zhang, and Tim 
> Harper.
> 
> 
> Bugs: MESOS-10115
> https://issues.apache.org/jira/browse/MESOS-10115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a new document containing an introduction for
> scheduler authors to the available methods for running
> workloads in a Mesos cluster.
> 
> 
> Diffs
> -
> 
>   docs/home.md 027319fa28abbdac21588ad1c62dae588033f60e 
>   docs/running-workloads.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72409/diff/2/
> 
> 
> Testing
> ---
> 
> Viewed with `site/mesos-website-dev.sh`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72409: Added intro docs on running workloads.

[Qian Zhang]

> On April 22, 2020, 10:09 p.m., Qian Zhang wrote:
> > docs/running-workloads.md
> > Lines 65-67 (patched)
> > 
> >
> > Do we want to mention how we set OOM score adjustment for burstable 
> > tasks somewhere in the doc?
> > 
> > And I think we also need to describe the newly introduced 
> > `REASON_CONTAINER_MEMORY_REQUEST_EXCEEDED` in the doc 
> > `task-state-reasons.md`.
> 
> Greg Mann wrote:
> I added these to https://reviews.apache.org/r/72410/ as well

Yeah, I see you added how we set OOM score adjustment in 
`nested-container-and-task-group.md` which is specific for nested container, 
but we also set OOM score adjustment for burstable command tasks.


- Qian


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


On April 28, 2020, 7:57 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72409/
> ---
> 
> (Updated April 28, 2020, 7:57 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Benjamin Mahler, Qian Zhang, and Tim 
> Harper.
> 
> 
> Bugs: MESOS-10115
> https://issues.apache.org/jira/browse/MESOS-10115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a new document containing an introduction for
> scheduler authors to the available methods for running
> workloads in a Mesos cluster.
> 
> 
> Diffs
> -
> 
>   docs/home.md 027319fa28abbdac21588ad1c62dae588033f60e 
>   docs/running-workloads.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72409/diff/2/
> 
> 
> Testing
> ---
> 
> Viewed with `site/mesos-website-dev.sh`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72410: Updated existing documentation for resource limits.

[Qian Zhang]

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




docs/task-state-reasons.md
Lines 126-133 (patched)


If we revert the changes about `REASON_CONTAINER_MEMORY_REQUEST_EXCEEDED` 
in https://reviews.apache.org/r/72442 , then we do not need to update this doc.



docs/upgrades.md
Lines 52 (patched)


I think we'd better to have a general description about the task resource 
limits feature (like how [quota 
limits](https://github.com/apache/mesos/blob/master/docs/upgrades.md#1-9-x-quota-guarantees)
 feature was described) rather than just mention CPU limits here.


- Qian Zhang


On April 28, 2020, 8:59 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72410/
> ---
> 
> (Updated April 28, 2020, 8:59 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Benjamin Mahler, Qian Zhang, and Tim 
> Harper.
> 
> 
> Bugs: MESOS-10115
> https://issues.apache.org/jira/browse/MESOS-10115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated existing documentation for resource limits.
> 
> 
> Diffs
> -
> 
>   docs/nested-container-and-task-group.md 
> 3325ff8e3a694ede82e22b24ab258742746c0bb7 
>   docs/scheduler-http-api.md 9831d527cc1f832a6fb0d0d330ebdc2a0b0f3774 
>   docs/task-state-reasons.md 10cb7e08d854ecfd16152fd370e4efe03b327347 
>   docs/upgrades.md 1e73e3d2626b7de93f5907bb52e008830f55e3a0 
> 
> 
> Diff: https://reviews.apache.org/r/72410/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 72445: Fixed operator api event inconsistencies issue.

[Dong Zhu]

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

Review request for mesos and Andrei Sekretenko.


Repository: mesos


Description
---

This patch intends to fix issue MESOS-10085.

When the authorization failed happens master return nothing to the
subscriber, subscriber isn't aware of what is happening, this issue
can lead to inconsistencies in Event stream.

Signed-off-by: Dong Zhu 


Diffs
-

  include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 
  include/mesos/v1/master/master.proto 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d 
  src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c 


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


Testing
---


Thanks,

Dong Zhu

Re: Review Request 72408: Made the scheduler resources a top-level item in the documentation.

[Qian Zhang]

> On April 22, 2020, 5:07 p.m., Qian Zhang wrote:
> > docs/home.md
> > Line 76 (original), 76 (patched)
> > 
> >
> > Should we name it `Frameworks`?
> 
> Greg Mann wrote:
> I went with Ben's suggestion, let me know what you think!

Yeah, I am OK with it.


- Qian


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


On April 28, 2020, 7:55 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72408/
> ---
> 
> (Updated April 28, 2020, 7:55 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Benjamin Mahler, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the scheduler resources a top-level item in the documentation.
> 
> 
> Diffs
> -
> 
>   docs/home.md 027319fa28abbdac21588ad1c62dae588033f60e 
> 
> 
> Diff: https://reviews.apache.org/r/72408/diff/2/
> 
> 
> Testing
> ---
> 
> Viewed using `site/mesos-website-dev.sh`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72445: Fixed operator api event inconsistencies issue.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [72445]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:16.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose 
--disable-libtool-wrappers --disable-parallel-test-execution' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/jenkins/buildbot.sh 2>&1 | 
tee build_72445"]

Error:
..
DPACKAGE_STRING=\"mesos\ 1.10.0\" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" 
-DPACKAGE=\"mesos\" -DVERSION=\"1.10.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 
-DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 
-DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 
-DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 
-DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 
-DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 -DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 
-DMESOS_HAS_JAVA=1 -DENABLE_NVML=1 -DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 
-DHAVE_LIBSVN_SUBR_1=1 -DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 
-DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 -DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. 
-I../../../src   -Werror -DLIBDIR=\"/tmp/SRC/build/mesos-1.10.0/_inst/lib\" 
-DPKGLIBEXECDIR=\"/tmp/SRC/build/mesos-1.10.0/_inst/libexec/mesos\" 
-DPKGDATADIR=\"/tmp/SRC/build/mesos-1.10.0/_inst/share/mesos\" 
-DPKGMODULEDIR=\"/tmp/SRC/build/mesos-1.10.0/_inst/lib/mesos/modules
 \" -I../../../include -I../include -I../include/mesos -D__STDC_FORMAT_MACROS 
-I../3rdparty/boost-1.65.0 -I../3rdparty/concurrentqueue-7b69a8f 
-I../3rdparty/elfio-3.2 -I../3rdparty/glog-0.4.0/src 
-I../3rdparty/grpc-1.10.0/include -I../3rdparty/leveldb-1.19/include 
-I../3rdparty/libarchive-3.3.2/libarchive/ 
-I../../../3rdparty/libprocess/include  -I../3rdparty/nvml-352.79 
-I../3rdparty/picojson-1.3.0 -I../3rdparty/protobuf-3.5.0/src 
-I../3rdparty/rapidjson-1.1.0/include -I../../../3rdparty/stout/include 
-I../3rdparty/zookeeper-3.4.8/src/c/include 
-I../3rdparty/zookeeper-3.4.8/src/c/generated 
-DSOURCE_DIR=\"/tmp/SRC/build/mesos-1.10.0/_build/sub/../..\" 
-DBUILD_DIR=\"/tmp/SRC/build/mesos-1.10.0/_build/sub\" 
-I../3rdparty/googletest-release-1.8.0/googletest/include 
-I../3rdparty/googletest-release-1.8.0/googlemock/include 
-DTESTLIBEXECDIR=\"/tmp/SRC/build/mesos-1.10.0/_inst/libexec/mesos/tests\" 
-DSBINDIR=\"/tmp/SRC/build/mesos-1.10.0/_inst/sbin\" 
-I/usr/lib/jvm/java-8-openjdk-amd64/inc
 lude -I/usr/lib/jvm/java-8-openjdk-amd64/include/linux 
-DZOOKEEPER_VERSION=\"3.4.8\" -I/usr/include/subversion-1 -I/usr/include/apr-1 
-I/usr/include/apr-1.0  -pthread -Wall -Wsign-compare -Wformat-security 
-fstack-protector-strong -fPIC -fPIE -g1 -O0 -Wno-unused-local-typedefs 
-std=c++11 -c -o tests/containerizer/mesos_tests-runtime_isolator_tests.o `test 
-f 'tests/containerizer/runtime_isolator_tests.cpp' || echo 
'../../../src/'`tests/containerizer/runtime_isolator_tests.cpp
g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.10.0\" -DPACKAGE_STRING=\"mesos\ 1.10.0\" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.10.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 
-DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 
-DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 
-DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 
-DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 -DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 
-DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 -DMESOS_HAS_JAVA=1 -DENABLE_NVML=1 
-DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 -DHAVE_LIBSVN_SUBR_1=1 
-DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 -DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 
-DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. -I../../../src   -Werror 
-DLIBDIR=\"/tmp/SRC/build/mesos-1.10.0/_inst/lib\" 
-DPKGLIBEXECDIR=\"/tmp/SRC/build/mesos-1.10.0/_inst/libexec/mesos\" 
-DPKGDATADIR=\"/tmp/SRC/build/mesos-1.10.0/_
 inst/share/mesos\" 
-DPKGMODULEDIR=\"/tmp/SRC/build/mesos-1.10.0/_inst/lib/mesos/modules\" 
-I../../../include -I../include -I../include/mesos -D__STDC_FORMAT_MACROS 
-I../3rdparty/boost-1.65.0 -I../3rdparty/concurrentqueue-7b69a8f 
-I../3rdparty/elfio-3.2 -I../3rdparty/glog-0.4.0/src 
-I../3rdparty/grpc-1.10.0/include -I../3rdparty/leveldb-1.19/include 
-I../3rdparty/libarchive-3.3.2/libarchive/ 
-I../../../3rdparty/libprocess/include  -I../3rdparty/nvml-352.79 
-I../3rdparty/picojson-1.3.0 -I../3rdparty/protobuf-3.5.0/src 
-I../3rdparty/rapidjson-1.1.0/include -I../../../3rdparty/stout/include 
-I../3rdparty/zookeeper-3.4.8/src/c/include 
-I../3rdparty/zookeeper-3.4.8/src/c/generated 
-DSOURCE_DIR=\"/tmp/SRC/build/mesos-1.10.0/_build/sub/../..\" 
-DBUILD_DIR=\"/tmp/SRC/build/mesos-1.10.0/_build/sub\" 
-I../3rdparty/googletest-release-1.8.0/googletest/include