Re: Review Request 66144: Enforced task launch order on the agent.

[Greg Mann]

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




src/slave/slave.hpp
Lines 1147-1155 (patched)


Before you mention the lifecycle, could you explain the purpose of these 
sequences?

i.e.,

```
// Sequences in this map are used to enforce the order of tasks launched on 
each executor.
//
// Note on the lifecycle of the sequence: ...
```



src/slave/slave.cpp
Lines 2227-2230 (patched)


Is it true that "we do not want the discard event to propagate to other 
tasks"?
I might recommend the following:

"We use this sequence only to maintain the task launching order. If the 
sequence is deleted, we do not want the resulting discard event to propagate up 
the chain, which would prevent the previous `.then()` or `.repair()` callbacks 
from being invoked. Thus, we use `undiscardable` to protect each `taskLaunch`."



src/slave/slave.cpp
Lines 2233-2258 (patched)


I would recommend moving this comment block above the `.onAny( ... )` call, 
since it is explaining the reason for the `.onAny` call and the conditions on 
`seqFuture`.

Then, it would be good to add a comment above `taskLaunch.onReady( ... )` 
explaining why we are chaining onto `taskLaunch` here, rather than dispatching 
directly. Maybe something like:

```
// We only want to execute the following callbacks once the work performed 
in the `taskLaunch` chain is complete. Thus, we add them onto the `taskLaunch` 
chain rather than dispatching directly.
taskLaunch
  .onReady( ...
```



src/slave/slave.cpp
Lines 2242 (patched)


s/task's/tasks'/



src/slave/slave.cpp
Lines 2251 (patched)


s/tasks'/task's/



src/slave/slave.cpp
Line 2234 (original), 2279 (patched)


s/expects new/expects a new/

s/task launch/task/



src/slave/slave.cpp
Line 2236 (original), 2281 (patched)


Use backticks instead of single quotes around `ExitedExecutorMessage`.



src/slave/slave.cpp
Lines 2286 (patched)


If `framework_ == nullptr` is true, then dereferencing the pointer here 
will be bad. You could enclose this in a conditional check, `if (framework_ != 
nullptr)`.


- Greg Mann


On March 29, 2018, 6:15 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66144/
> ---
> 
> (Updated March 29, 2018, 6:15 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8617 and MESOS-8624
> https://issues.apache.org/jira/browse/MESOS-8617
> https://issues.apache.org/jira/browse/MESOS-8624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Up until now, Mesos does not guarantee in-order
> task launch on the agent. There are two asynchronous
> steps (unschedule GC and task authorization) in the
> agent task launch path. Depending on the CPU scheduling
> order, a later task launch may finish these two steps earlier
> than its predecessors and get to the launch executor stage
> earlier, resulting in out-of-order task delivery.
> 
> This patch enforces the task delivery order by sequencing
> task launch after the two asynchronous steps, specifically
> right before entering `__run()`.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 37f0361251524e63d02d251e8a03901812b8affb 
>   src/slave/slave.cpp a4bd4ccd3fc59c3c0e462d9b480f5424b3e52d7a 
> 
> 
> Diff: https://reviews.apache.org/r/66144/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66143: Refactored agent task launch for better composition [2/2].

[Greg Mann]

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




src/slave/slave.cpp
Line 2215 (original), 2220-2227 (patched)


Not indented far enough.



src/slave/slave.cpp
Lines 2328-2335 (original), 2341-2371 (patched)


Should be indented 2 more spaces.


- Greg Mann


On March 28, 2018, 1:07 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66143/
> ---
> 
> (Updated March 28, 2018, 1:07 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8617 and MESOS-8624
> https://issues.apache.org/jira/browse/MESOS-8617
> https://issues.apache.org/jira/browse/MESOS-8624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helps to encapsulate a task launch into a single
> future which will come in handy when enforcing the task
> launch order.
> 
> Affected tests are also updated.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 37f0361251524e63d02d251e8a03901812b8affb 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
>   src/tests/mock_slave.hpp 8ec2b65a75f774d970ef6507df8b0c02fda5f2fd 
>   src/tests/mock_slave.cpp 8ec55b6aa84dd8b1c3743cbcdc246311585d0f63 
>   src/tests/slave_tests.cpp 0d7d9726ee39e4c588ea5eb8a54a73fbe1249353 
> 
> 
> Diff: https://reviews.apache.org/r/66143/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66126: Refactored agent task launch for better composition [1/2].

[Greg Mann]

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




src/slave/slave.cpp
Lines 2153-2201 (patched)


Should be indented 2 more spaces.


- Greg Mann


On March 28, 2018, 1:05 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66126/
> ---
> 
> (Updated March 28, 2018, 1:05 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8617 and MESOS-8624
> https://issues.apache.org/jira/browse/MESOS-8617
> https://issues.apache.org/jira/browse/MESOS-8624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helps to encapsulate a task launch into a single
> future which will come in handy when enforcing the task
> launch order.
> 
> This patch also consolidated the error handling code
> in the task launch path.
> 
> Affected tests are also updated.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 37f0361251524e63d02d251e8a03901812b8affb 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
>   src/tests/mock_slave.hpp 8ec2b65a75f774d970ef6507df8b0c02fda5f2fd 
>   src/tests/mock_slave.cpp 8ec55b6aa84dd8b1c3743cbcdc246311585d0f63 
>   src/tests/slave_tests.cpp 0d7d9726ee39e4c588ea5eb8a54a73fbe1249353 
> 
> 
> Diff: https://reviews.apache.org/r/66126/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66289: Added fields to GET_STATE necessary for streaming Web UI.

[Benjamin Mahler]

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



Thanks Armand, can you add greg to the review?


include/mesos/master/master.proto
Lines 335-336 (patched)


Exposing this makes me wonder what happens when this master loses 
leadership? It's not really a new question, but seems like the streaming API 
needs to do something when leadership is lost or if the master is not the 
leader in the first place? Do we support streaming against non-leaders?


- Benjamin Mahler


On March 27, 2018, 12:10 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66289/
> ---
> 
> (Updated March 27, 2018, 12:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added fields to GET_STATE necessary for streaming Web UI.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md 10dcac83fa70da5760a5c231665d7498cc168622 
>   include/mesos/master/master.proto aa63904a33290a3beda162bbc9f44b56ab04a1e7 
>   include/mesos/v1/master/master.proto 
> ddb28f96b2a3a439bb9a829995a9a3015f65ba43 
>   src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
> 
> 
> Diff: https://reviews.apache.org/r/66289/diff/1/
> 
> 
> Testing
> ---
> 
> This is an updated version of 
> https://github.com/benh/mesos/commit/c3ce5edb35e10e682f7d791719ad077f78b1dc95.
> 
> I have applied a `make check` + run a master locally and done a `curl -d 
> '{"type":"GET_STATE"}' -H "Content-Type: application/json" -X POST 
> http://localhost:5061/api/v1` to check the JSON returned:
> ```
> {
> "type": "GET_STATE",
> "get_state": {
> "get_tasks": {},
> "get_executors": {},
> "get_frameworks": {},
> "get_agents": {},
> "version": "1.6.0",
> "build_time": {
> "nanoseconds": 15220714190
> },
> "build_user": "agrillet",
> "start_time": {
> "nanoseconds": 1522078049371355904
> },
> "leader": {
> "id": "1ce9804a-efd6-422a-8669-ce82e4183173",
> "ip": 3492307904,
> "port": 5061,
> "pid": "master@192.99.40.208:5061",
> "hostname": "gru1.hw.ca1.mesosphere.com",
> "version": "1.6.0",
> "address": {
> "hostname": "gru1.hw.ca1.mesosphere.com",
> "ip": "192.99.40.208",
> "port": 5061
> },
> "capabilities": [
> {
> "type": "AGENT_UPDATE"
> }
> ]
> },
> "elected_time": {
> "nanoseconds": 1522078049390997760
> }
> }
> }
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 66323: Added tests for failed task launch on agent.

[Meng Zhu]

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

(Updated March 30, 2018, 11:10 a.m.)


Review request for mesos and Greg Mann.


Changes
---

Added one task, three tasks instead of two to cover more corner cases.


Bugs: MESOS-8617 and MESOS-8624
https://issues.apache.org/jira/browse/MESOS-8617
https://issues.apache.org/jira/browse/MESOS-8624


Repository: mesos


Description
---

This test verifies the agent behavior of launching
two task(group)s using the same executor. When both
tasks are launching on the agent (before creating
any executor), if the first task (in the agent receiving
order) fails to launch, the later task will get dropped.
If the later task fails to launch, the first task
should still launch successfully.


Diffs (updated)
-

  src/tests/slave_tests.cpp 0d7d9726ee39e4c588ea5eb8a54a73fbe1249353 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 66144: Enforced task launch order on the agent.

[Meng Zhu]

> On March 28, 2018, 11:11 p.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 2261-2268 (patched)
> > 
> >
> > I think we want to `return;` at the end of this conditional block? And 
> > if you make this lambda accept a string parameter, you could include the 
> > Failure message in this logging.
> > 
> > Also, it looks like the local variable `error` is not really needed?

We need to continue. 
https://github.com/apache/mesos/blob/7e58e14de74a5dd39b167d6698e79e29cdd24ac6/src/slave/slave.cpp#L2193-L2204

The failure message must have been printed early, let's print it again here.

Removed `error`.


- Meng


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


On March 29, 2018, 11:15 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66144/
> ---
> 
> (Updated March 29, 2018, 11:15 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8617 and MESOS-8624
> https://issues.apache.org/jira/browse/MESOS-8617
> https://issues.apache.org/jira/browse/MESOS-8624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Up until now, Mesos does not guarantee in-order
> task launch on the agent. There are two asynchronous
> steps (unschedule GC and task authorization) in the
> agent task launch path. Depending on the CPU scheduling
> order, a later task launch may finish these two steps earlier
> than its predecessors and get to the launch executor stage
> earlier, resulting in out-of-order task delivery.
> 
> This patch enforces the task delivery order by sequencing
> task launch after the two asynchronous steps, specifically
> right before entering `__run()`.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 37f0361251524e63d02d251e8a03901812b8affb 
>   src/slave/slave.cpp a4bd4ccd3fc59c3c0e462d9b480f5424b3e52d7a 
> 
> 
> Diff: https://reviews.apache.org/r/66144/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66371: Temporarily disabled CSI proto compilation in CMake.

[Chun-Hung Hsiao]

> On March 30, 2018, 10:02 a.m., David Forsythe wrote:
> > This works for me on FreeBSD now, though I'm not sure if it would be better 
> > to gate this rather than removing it completely?

I remove it instead of gating it for the following reasons:

1. Current CMake doesn't include any build rules for the components (storage 
local resource provider) that depends on CSI, so it doesn't matter if we 
compile the proto file. The rules will be introduced in 
https://reviews.apache.org/r/66163/.
2. The compilation problem happens not only on FreeBSD, but also on platforms 
using GCC 7, so there is no simple gate for this.
3. This is just a temporary removal. https://reviews.apache.org/r/65594/ will 
introduce a internal proto message that uses a CSI message, so eventually we 
need CSI proto to be compiled on all supported platforms, after the CSI bundle 
is bumped to 0.2 (where there will be no name conflicts).

Therefore, it doesn't worth the time to design a gate that will be removed soon.


- Chun-Hung


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


On March 29, 2018, 11:58 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66371/
> ---
> 
> (Updated March 29, 2018, 11:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, David Forsythe, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8749
> https://issues.apache.org/jira/browse/MESOS-8749
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `major` and `minor` macros defined on some systems conflict with
> field names in the CSI v0.1.0 spec proto. Temporarily disable it in
> CMake until CSI is bumped to 0.2.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt fb9e9d4cfc4a62830fe3065a139ae14401c0e52e 
> 
> 
> Diff: https://reviews.apache.org/r/66371/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` with CMake
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66367: Made the master drop `RECONCILE_OPERATIONS` calls from v0 schedulers.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 29, 2018, 5:23 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66367/
> ---
> 
> (Updated March 29, 2018, 5:23 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the master drop `RECONCILE_OPERATIONS` calls from v0 schedulers.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 
> 
> 
> Diff: https://reviews.apache.org/r/66367/diff/1/
> 
> 
> Testing
> ---
> 
> Existing tests pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66366: Fixed generation of operation status updates in `Master::_apply()`.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 29, 2018, 5:19 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66366/
> ---
> 
> (Updated March 29, 2018, 5:19 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates `Master::_apply() ` to make it include operation ID
> in the initial operation status of operations that have an ID.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 
> 
> 
> Diff: https://reviews.apache.org/r/66366/diff/1/
> 
> 
> Testing
> ---
> 
> Tests still pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66365: Fixed generation of operation statuses in `Slave::applyOperation()`.

[Greg Mann]

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


Fix it, then Ship it!





src/slave/slave.cpp
Lines 4266-4268 (patched)


Was this formatting produced by clang-format?

I would probably do:
```
  Option operationId =
message.operation_info().has_id()
  ? message.operation_info().id()
  : Option::none();
```
but if clang-format generated this, then let's just keep it.


- Greg Mann


On March 29, 2018, 5:19 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66365/
> ---
> 
> (Updated March 29, 2018, 5:19 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates `Slave::applyOperation()` to make it include the
> operation ID in operation statuses for operations that have an ID.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
> 
> 
> Diff: https://reviews.apache.org/r/66365/diff/1/
> 
> 
> Testing
> ---
> 
> Tests still pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66364: Added missing `type` field to the `scheduler::Response` protos.

[Greg Mann]

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


Ship it!




lol

- Greg Mann


On March 29, 2018, 5:18 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66364/
> ---
> 
> (Updated March 29, 2018, 5:18 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added missing `type` field to the `scheduler::Response` protos.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> 339a39776f8b730c4fa6bf12d1ed490a1ad858e6 
>   include/mesos/v1/scheduler/scheduler.proto 
> f08fdfbab2a13d1104e7be4b42bf20e9048ab562 
> 
> 
> Diff: https://reviews.apache.org/r/66364/diff/1/
> 
> 
> Testing
> ---
> 
> Mesos still compiles on GNU/Linux =).
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66363: Removed unnecessary code and a misleading comment from a test.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 29, 2018, 5:18 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66363/
> ---
> 
> (Updated March 29, 2018, 5:18 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unnecessary code and a misleading comment from a test.
> 
> 
> Diffs
> -
> 
>   src/tests/reconciliation_tests.cpp ad6c99306a82ef3da54a9d6d3c9d6f11ba145d6c 
> 
> 
> Diff: https://reviews.apache.org/r/66363/diff/1/
> 
> 
> Testing
> ---
> 
> Test still passed on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66362: Fixed use of const reference for storing an rvalue in a test.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 29, 2018, 5:17 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66362/
> ---
> 
> (Updated March 29, 2018, 5:17 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed use of const reference for storing an rvalue in a test.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 98b697d8f8f249b801f960dab9cef5958f920302 
> 
> 
> Diff: https://reviews.apache.org/r/66362/diff/1/
> 
> 
> Testing
> ---
> 
> Tests passed on Mesosphere's internal CI.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66371: Temporarily disabled CSI proto compilation in CMake.

[David Forsythe]

> On March 30, 2018, 8:34 a.m., David Forsythe wrote:
> > src/CMakeLists.txt
> > Line 35 (original)
> > 
> >
> > Should this line be removed?
> 
> Chun-Hung Hsiao wrote:
> This line is the same as Line 31. The review board is not clever enough 
> to mark the intended one ;)

Ah, of course.


- David


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


On March 29, 2018, 11:58 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66371/
> ---
> 
> (Updated March 29, 2018, 11:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, David Forsythe, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8749
> https://issues.apache.org/jira/browse/MESOS-8749
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `major` and `minor` macros defined on some systems conflict with
> field names in the CSI v0.1.0 spec proto. Temporarily disable it in
> CMake until CSI is bumped to 0.2.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt fb9e9d4cfc4a62830fe3065a139ae14401c0e52e 
> 
> 
> Diff: https://reviews.apache.org/r/66371/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` with CMake
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66371: Temporarily disabled CSI proto compilation in CMake.

[David Forsythe]

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


Ship it!




This works for me on FreeBSD now, though I'm not sure if it would be better to 
gate this rather than removing it completely?

- David Forsythe


On March 29, 2018, 11:58 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66371/
> ---
> 
> (Updated March 29, 2018, 11:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, David Forsythe, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8749
> https://issues.apache.org/jira/browse/MESOS-8749
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `major` and `minor` macros defined on some systems conflict with
> field names in the CSI v0.1.0 spec proto. Temporarily disable it in
> CMake until CSI is bumped to 0.2.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt fb9e9d4cfc4a62830fe3065a139ae14401c0e52e 
> 
> 
> Diff: https://reviews.apache.org/r/66371/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` with CMake
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66371: Temporarily disabled CSI proto compilation in CMake.

[Chun-Hung Hsiao]

> On March 30, 2018, 8:34 a.m., David Forsythe wrote:
> > src/CMakeLists.txt
> > Line 35 (original)
> > 
> >
> > Should this line be removed?

This line is the same as Line 31. The review board is not clever enough to mark 
the intended one ;)


- Chun-Hung


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


On March 29, 2018, 11:58 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66371/
> ---
> 
> (Updated March 29, 2018, 11:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, David Forsythe, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8749
> https://issues.apache.org/jira/browse/MESOS-8749
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `major` and `minor` macros defined on some systems conflict with
> field names in the CSI v0.1.0 spec proto. Temporarily disable it in
> CMake until CSI is bumped to 0.2.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt fb9e9d4cfc4a62830fe3065a139ae14401c0e52e 
> 
> 
> Diff: https://reviews.apache.org/r/66371/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` with CMake
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66371: Temporarily disabled CSI proto compilation in CMake.

[David Forsythe]

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




src/CMakeLists.txt
Line 35 (original)


Should this line be removed?


- David Forsythe


On March 29, 2018, 11:58 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66371/
> ---
> 
> (Updated March 29, 2018, 11:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, David Forsythe, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8749
> https://issues.apache.org/jira/browse/MESOS-8749
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `major` and `minor` macros defined on some systems conflict with
> field names in the CSI v0.1.0 spec proto. Temporarily disable it in
> CMake until CSI is bumped to 0.2.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt fb9e9d4cfc4a62830fe3065a139ae14401c0e52e 
> 
> 
> Diff: https://reviews.apache.org/r/66371/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` with CMake
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>