Re: Review Request 66577: Enabled CSI proto compilation by default.

[Chun-Hung Hsiao]

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

(Updated April 20, 2018, 11:34 p.m.)


Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.


Changes
---

Addressed Benjamin's comment.


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


Repository: mesos


Description
---

CSI proto compilation is disabled due to MESOS-8749, which is resolved
by bumping CSI to v0.2. This patch enables the compilation again.


Diffs (updated)
-

  src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
  src/Makefile.am 9d610bbe8108e5caa4a22977caa9dff07c8bb665 
  src/slave/flags.cpp e330e5fa0b49ad1d1765e492bc44a262a7856db5 
  src/slave/slave.cpp 9d2d1928b231044988f1855eb518448db38ff04f 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Chun-Hung Hsiao]

> On April 20, 2018, 7:53 a.m., Benjamin Bannier wrote:
> > src/Makefile.am
> > Lines 1529-1536 (original), 1521-1525 (patched)
> > 
> >
> > We can merge these two blocks now.
> 
> Chun-Hung Hsiao wrote:
> I was following the convention to split apart implementation files and 
> header files ;) But will do.

Oh I guess you mean to just remove the blank line. I'll modify my code this way.


- Chun-Hung


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


On April 17, 2018, 3:03 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 17, 2018, 3:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8749, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
>   src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
>   src/slave/flags.cpp 4ea2286a6a874e6e7f473f97fb0431e17b19c1f8 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Chun-Hung Hsiao]

> On April 20, 2018, 7:53 a.m., Benjamin Bannier wrote:
> > src/Makefile.am
> > Lines 1529-1536 (original), 1521-1525 (patched)
> > 
> >
> > We can merge these two blocks now.

I was following the convention to split apart implementation files and header 
files ;) But will do.


- Chun-Hung


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


On April 17, 2018, 3:03 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 17, 2018, 3:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8749, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
>   src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
>   src/slave/flags.cpp 4ea2286a6a874e6e7f473f97fb0431e17b19c1f8 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/Makefile.am
Lines 1529-1536 (original), 1521-1525 (patched)


We can merge these two blocks now.


- Benjamin Bannier


On April 17, 2018, 5:03 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 17, 2018, 5:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8749, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
>   src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
>   src/slave/flags.cpp 4ea2286a6a874e6e7f473f97fb0431e17b19c1f8 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Chun-Hung Hsiao]

> On April 17, 2018, 6:19 p.m., Benno Evers wrote:
> > src/Makefile.am
> > Line 2378 (original), 2369 (patched)
> > 
> >
> > Will this library also get built when using `cmake`?

Please see https://reviews.apache.org/r/66163/. Enablement of SLRP in CMake are 
in the following patches in the chain.


> On April 17, 2018, 6:19 p.m., Benno Evers wrote:
> > src/Makefile.am
> > Lines 2498 (patched)
> > 
> >
> > This seems to be missing in the `CMakeLists.txt`?

Same here. We haven't enabled SLRP in CMake yet before this patch.


- Chun-Hung


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


On April 17, 2018, 3:03 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 17, 2018, 3:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8749, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
>   src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
>   src/slave/flags.cpp 4ea2286a6a874e6e7f473f97fb0431e17b19c1f8 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Benno Evers]

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


Fix it, then Ship it!





src/Makefile.am
Line 2378 (original), 2369 (patched)


Will this library also get built when using `cmake`?



src/Makefile.am
Lines 2498 (patched)


This seems to be missing in the `CMakeLists.txt`?


- Benno Evers


On April 17, 2018, 3:03 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 17, 2018, 3:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8749, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
>   src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
>   src/slave/flags.cpp 4ea2286a6a874e6e7f473f97fb0431e17b19c1f8 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Chun-Hung Hsiao]

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

(Updated April 17, 2018, 3:03 a.m.)


Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

CSI proto compilation is disabled due to MESOS-8749, which is resolved
by bumping CSI to v0.2. This patch enables the compilation again.


Diffs (updated)
-

  src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
  src/Makefile.am 07eb13851967b8a28b46afa3304a9d4e298a5016 
  src/slave/flags.cpp 4ea2286a6a874e6e7f473f97fb0431e17b19c1f8 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Andrew Schwartzmeyer]

> On April 12, 2018, 9:44 a.m., Andrew Schwartzmeyer wrote:
> > src/CMakeLists.txt
> > Lines 32-35 (patched)
> > 
> >
> > Nit: can this be moved down so it goes "no options", "grpc option", 
> > "java option", "internal option."
> 
> Chun-Hung Hsiao wrote:
> I put it in this order because this is from the 3rdparty bundle and 
> (although there is no public Mesos proto that uses CSI for now) this opens 
> the possiblity to let a public Mesos proto to use CSI.

Gotcha.


- Andrew


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


On April 11, 2018, 6:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 11, 2018, 6:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8724, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/Makefile.am 257ff0e8d21e5240c27a123ed0cd563214e24fce 
>   src/slave/flags.cpp bdfc49a3903899b2741bb60c7e9e89f0196492e4 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Chun-Hung Hsiao]

> On April 12, 2018, 4:44 p.m., Andrew Schwartzmeyer wrote:
> > src/CMakeLists.txt
> > Lines 32-35 (patched)
> > 
> >
> > Nit: can this be moved down so it goes "no options", "grpc option", 
> > "java option", "internal option."

I put it in this order because this is from the 3rdparty bundle and (although 
there is no public Mesos proto that uses CSI for now) this opens the possiblity 
to let a public Mesos proto to use CSI.


- Chun-Hung


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


On April 12, 2018, 1:39 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 12, 2018, 1:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8724, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/Makefile.am 257ff0e8d21e5240c27a123ed0cd563214e24fce 
>   src/slave/flags.cpp bdfc49a3903899b2741bb60c7e9e89f0196492e4 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Andrew Schwartzmeyer]

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



Note: I did not review the Autotools part at all ;)


src/CMakeLists.txt
Lines 32-35 (patched)


Nit: can this be moved down so it goes "no options", "grpc option", "java 
option", "internal option."


- Andrew Schwartzmeyer


On April 11, 2018, 6:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 11, 2018, 6:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8724, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/Makefile.am 257ff0e8d21e5240c27a123ed0cd563214e24fce 
>   src/slave/flags.cpp bdfc49a3903899b2741bb60c7e9e89f0196492e4 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66577: Enabled CSI proto compilation by default.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 66398.

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

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

Relevant logs:

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

```
error: missing binary patch data for '3rdparty/csi-0.2.0.tar.gz'
error: binary patch does not apply to '3rdparty/csi-0.2.0.tar.gz'
error: 3rdparty/csi-0.2.0.tar.gz: patch does not apply
```

- Mesos Reviewbot Windows


On April 12, 2018, 1:39 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66577/
> ---
> 
> (Updated April 12, 2018, 1:39 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.
> 
> 
> Bugs: MESOS-8724
> https://issues.apache.org/jira/browse/MESOS-8724
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CSI proto compilation is disabled due to MESOS-8724, which is resolved
> by bumping CSI to v0.2. This patch enables the compilation again.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
>   src/Makefile.am 257ff0e8d21e5240c27a123ed0cd563214e24fce 
>   src/slave/flags.cpp bdfc49a3903899b2741bb60c7e9e89f0196492e4 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66577/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 66577: Enabled CSI proto compilation by default.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier, Benno Evers, and Jie Yu.


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


Repository: mesos


Description
---

CSI proto compilation is disabled due to MESOS-8724, which is resolved
by bumping CSI to v0.2. This patch enables the compilation again.


Diffs
-

  src/CMakeLists.txt 6fc45dba8ee70b591be03ac483655c1844a0a6b9 
  src/Makefile.am 257ff0e8d21e5240c27a123ed0cd563214e24fce 
  src/slave/flags.cpp bdfc49a3903899b2741bb60c7e9e89f0196492e4 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao