Re: Review Request 67779: Introduced `CREATE_DISK` and `DESTROY_DISK` operations.

[Chun-Hung Hsiao]

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

(Updated July 12, 2018, 8:13 p.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, James 
DeFelice, and Jie Yu.


Changes
---

Fixed a typo.


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


Repository: mesos


Description
---

This patch introduces `CREATE_DISK` and `DESTROY_DISK` operations for
conversion between RAW disk resaurces and MOUNT/BLOCK disk resources to
replace `CREATE_VOLUME`/`DESTROY_VOLUME`/`CREATE_BLOCK`/`DESTROY_BLOCK`.

This replacement involves two aspects:

1. Renaming: The term "volume" is overloaded and created name conflicts
   in mesos. This renaming would lead to cleaner names in ACL and other
   places, and is more consistent with CSI.

2. Removing PATH support. PATH disks are used to split a disk resource
   into smaller chunks for concurrent independent uses. However, this
   concept does not quite fit CSI well.


Diffs (updated)
-

  include/mesos/mesos.proto 463e6adcdf14115b2f21270bb1fd9c45e1b67cc3 
  include/mesos/v1/mesos.proto 8eaad9c4b2a9cdd527922a004b0f7dd0dc58a7f1 


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

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


Testing
---

Test done later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 67779: Introduced `CREATE_DISK` and `DESTROY_DISK` operations.

[Jan Schlicht]

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


Fix it, then Ship it!





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


s/MONUT/MOUNT/



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


s/MONUT/MOUNT/


- Jan Schlicht


On July 12, 2018, 6:38 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67779/
> ---
> 
> (Updated July 12, 2018, 6:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, James 
> DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9066
> https://issues.apache.org/jira/browse/MESOS-9066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces `CREATE_DISK` and `DESTROY_DISK` operations for
> conversion between RAW disk resaurces and MOUNT/BLOCK disk resources to
> replace `CREATE_VOLUME`/`DESTROY_VOLUME`/`CREATE_BLOCK`/`DESTROY_BLOCK`.
> 
> This replacement involves two aspects:
> 
> 1. Renaming: The term "volume" is overloaded and created name conflicts
>in mesos. This renaming would lead to cleaner names in ACL and other
>places, and is more consistent with CSI.
> 
> 2. Removing PATH support. PATH disks are used to split a disk resource
>into smaller chunks for concurrent independent uses. However, this
>concept does not quite fit CSI well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 463e6adcdf14115b2f21270bb1fd9c45e1b67cc3 
>   include/mesos/v1/mesos.proto 8eaad9c4b2a9cdd527922a004b0f7dd0dc58a7f1 
> 
> 
> Diff: https://reviews.apache.org/r/67779/diff/3/
> 
> 
> Testing
> ---
> 
> Test done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67779: Introduced `CREATE_DISK` and `DESTROY_DISK` operations.

[Chun-Hung Hsiao]

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

(Updated July 12, 2018, 4:38 a.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, James 
DeFelice, and Jie Yu.


Changes
---

Addressed Benjamin's and Jan's comments.


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


Repository: mesos


Description
---

This patch introduces `CREATE_DISK` and `DESTROY_DISK` operations for
conversion between RAW disk resaurces and MOUNT/BLOCK disk resources to
replace `CREATE_VOLUME`/`DESTROY_VOLUME`/`CREATE_BLOCK`/`DESTROY_BLOCK`.

This replacement involves two aspects:

1. Renaming: The term "volume" is overloaded and created name conflicts
   in mesos. This renaming would lead to cleaner names in ACL and other
   places, and is more consistent with CSI.

2. Removing PATH support. PATH disks are used to split a disk resource
   into smaller chunks for concurrent independent uses. However, this
   concept does not quite fit CSI well.


Diffs (updated)
-

  include/mesos/mesos.proto 463e6adcdf14115b2f21270bb1fd9c45e1b67cc3 
  include/mesos/v1/mesos.proto 8eaad9c4b2a9cdd527922a004b0f7dd0dc58a7f1 


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

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


Testing
---

Test done later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 67779: Introduced `CREATE_DISK` and `DESTROY_DISK` operations.

[Chun-Hung Hsiao]

> On July 11, 2018, 12:15 p.m., Jan Schlicht wrote:
> > include/mesos/mesos.proto
> > Lines 2006 (patched)
> > 
> >
> > s/target/disk/

Somehow my response to this was dropped :(

I originally used `disk` here but this would result in the following 
weird-looking code:
```
operation.destroy_disk().disk().disk()
```

Benjamin suggested using `source` here to establish a general naming convention.


- Chun-Hung


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


On July 10, 2018, 3:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67779/
> ---
> 
> (Updated July 10, 2018, 3:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, James 
> DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9066
> https://issues.apache.org/jira/browse/MESOS-9066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces `CREATE_DISK` and `DESTROY_DISK` operations for
> conversion between RAW disk resaurces and MOUNT/BLOCK disk resources to
> replace `CREATE_VOLUME`/`DESTROY_VOLUME`/`CREATE_BLOCK`/`DESTROY_BLOCK`.
> 
> This replacement involves two aspects:
> 
> 1. Renaming: The term "volume" is overloaded and created name conflicts
>in mesos. This renaming would lead to cleaner names in ACL and other
>places, and is more consistent with CSI.
> 
> 2. Removing PATH support. PATH disks are used to split a disk resource
>into smaller chunks for concurrent independent uses. However, this
>concept does not quite fit CSI well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 463e6adcdf14115b2f21270bb1fd9c45e1b67cc3 
>   include/mesos/v1/mesos.proto 8eaad9c4b2a9cdd527922a004b0f7dd0dc58a7f1 
> 
> 
> Diff: https://reviews.apache.org/r/67779/diff/2/
> 
> 
> Testing
> ---
> 
> Test done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67779: Introduced `CREATE_DISK` and `DESTROY_DISK` operations.

[Chun-Hung Hsiao]

> On July 11, 2018, 12:47 p.m., Benjamin Bannier wrote:
> > include/mesos/mesos.proto
> > Lines 1927-1929 (original), 1927-1932 (patched)
> > 
> >
> > We don't seem to document removed proto definitions, maybe we should 
> > just drop this comment here and below.

Would that look weird for people that have no context about why some tag 
indices are not used?


> On July 11, 2018, 12:47 p.m., Benjamin Bannier wrote:
> > include/mesos/mesos.proto
> > Lines 1991 (patched)
> > 
> >
> > Use backticks around `RAW`, `MOUNT`, and `BLOCK`; here and below.

IIRC there are a couple places where we don't backtick them. But I could do it 
here.


> On July 11, 2018, 12:47 p.m., Benjamin Bannier wrote:
> > include/mesos/mesos.proto
> > Lines 2006 (patched)
> > 
> >
> > I agree with Jan that `target` is confusing as we don't want users to 
> > pass the target resource (i.e., an already "destroy disk resource") here. 
> > Calling this something else would make clear that we want the source 
> > resource of the operation passed in, not the expect result (contrary to how 
> > e.g., a reserve operation is structured).
> > 
> > Maybe even go with `source` like in the `Create` op?

This is also an option I thought about. Just that I'm not sure if "source" is a 
good objective for the verb "destroy." If you guys think "source" is better 
then I'll go with it.


- Chun-Hung


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


On July 10, 2018, 3:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67779/
> ---
> 
> (Updated July 10, 2018, 3:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, James 
> DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9066
> https://issues.apache.org/jira/browse/MESOS-9066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces `CREATE_DISK` and `DESTROY_DISK` operations for
> conversion between RAW disk resaurces and MOUNT/BLOCK disk resources to
> replace `CREATE_VOLUME`/`DESTROY_VOLUME`/`CREATE_BLOCK`/`DESTROY_BLOCK`.
> 
> This replacement involves two aspects:
> 
> 1. Renaming: The term "volume" is overloaded and created name conflicts
>in mesos. This renaming would lead to cleaner names in ACL and other
>places, and is more consistent with CSI.
> 
> 2. Removing PATH support. PATH disks are used to split a disk resource
>into smaller chunks for concurrent independent uses. However, this
>concept does not quite fit CSI well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 463e6adcdf14115b2f21270bb1fd9c45e1b67cc3 
>   include/mesos/v1/mesos.proto 8eaad9c4b2a9cdd527922a004b0f7dd0dc58a7f1 
> 
> 
> Diff: https://reviews.apache.org/r/67779/diff/2/
> 
> 
> Testing
> ---
> 
> Test done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67779: Introduced `CREATE_DISK` and `DESTROY_DISK` operations.

[Chun-Hung Hsiao]

> On July 11, 2018, 12:15 p.m., Jan Schlicht wrote:
> > include/mesos/mesos.proto
> > Lines 1991 (patched)
> > 
> >
> > Let's not say "convert" here, otherwise we could use a single 
> > `ConvertVolume` operation instead of create and destroy. How about "Create 
> > a MOUNT or BLOCK disk resource from a RAW disk resource"?

Good point. Will do.


- Chun-Hung


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


On July 10, 2018, 3:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67779/
> ---
> 
> (Updated July 10, 2018, 3:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, James 
> DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9066
> https://issues.apache.org/jira/browse/MESOS-9066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces `CREATE_DISK` and `DESTROY_DISK` operations for
> conversion between RAW disk resaurces and MOUNT/BLOCK disk resources to
> replace `CREATE_VOLUME`/`DESTROY_VOLUME`/`CREATE_BLOCK`/`DESTROY_BLOCK`.
> 
> This replacement involves two aspects:
> 
> 1. Renaming: The term "volume" is overloaded and created name conflicts
>in mesos. This renaming would lead to cleaner names in ACL and other
>places, and is more consistent with CSI.
> 
> 2. Removing PATH support. PATH disks are used to split a disk resource
>into smaller chunks for concurrent independent uses. However, this
>concept does not quite fit CSI well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 463e6adcdf14115b2f21270bb1fd9c45e1b67cc3 
>   include/mesos/v1/mesos.proto 8eaad9c4b2a9cdd527922a004b0f7dd0dc58a7f1 
> 
> 
> Diff: https://reviews.apache.org/r/67779/diff/2/
> 
> 
> Testing
> ---
> 
> Test done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67779: Introduced `CREATE_DISK` and `DESTROY_DISK` operations.

[Benjamin Bannier]

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




include/mesos/mesos.proto
Lines 1927-1929 (original), 1927-1932 (patched)


We don't seem to document removed proto definitions, maybe we should just 
drop this comment here and below.



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


Use backticks around `RAW`, `MOUNT`, and `BLOCK`; here and below.



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


I agree with Jan that `target` is confusing as we don't want users to pass 
the target resource (i.e., an already "destroy disk resource") here. Calling 
this something else would make clear that we want the source resource of the 
operation passed in, not the expect result (contrary to how e.g., a reserve 
operation is structured).

Maybe even go with `source` like in the `Create` op?


- Benjamin Bannier


On July 10, 2018, 5:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67779/
> ---
> 
> (Updated July 10, 2018, 5:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, James 
> DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9066
> https://issues.apache.org/jira/browse/MESOS-9066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces `CREATE_DISK` and `DESTROY_DISK` operations for
> conversion between RAW disk resaurces and MOUNT/BLOCK disk resources to
> replace `CREATE_VOLUME`/`DESTROY_VOLUME`/`CREATE_BLOCK`/`DESTROY_BLOCK`.
> 
> This replacement involves two aspects:
> 
> 1. Renaming: The term "volume" is overloaded and created name conflicts
>in mesos. This renaming would lead to cleaner names in ACL and other
>places, and is more consistent with CSI.
> 
> 2. Removing PATH support. PATH disks are used to split a disk resource
>into smaller chunks for concurrent independent uses. However, this
>concept does not quite fit CSI well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 463e6adcdf14115b2f21270bb1fd9c45e1b67cc3 
>   include/mesos/v1/mesos.proto 8eaad9c4b2a9cdd527922a004b0f7dd0dc58a7f1 
> 
> 
> Diff: https://reviews.apache.org/r/67779/diff/2/
> 
> 
> Testing
> ---
> 
> Test done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67779: Introduced `CREATE_DISK` and `DESTROY_DISK` operations.

[Jan Schlicht]

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




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


Let's not say "convert" here, otherwise we could use a single 
`ConvertVolume` operation instead of create and destroy. How about "Create a 
MOUNT or BLOCK disk resource from a RAW disk resource"?



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


See above. "Destroys a MOUNT or BLOCK disk resource. This will result in a 
RAW disk resource"?



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


s/target/disk/



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


See above.



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


See above.



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


s/target/disk/


- Jan Schlicht


On July 10, 2018, 5:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67779/
> ---
> 
> (Updated July 10, 2018, 5:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, James 
> DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9066
> https://issues.apache.org/jira/browse/MESOS-9066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces `CREATE_DISK` and `DESTROY_DISK` operations for
> conversion between RAW disk resaurces and MOUNT/BLOCK disk resources to
> replace `CREATE_VOLUME`/`DESTROY_VOLUME`/`CREATE_BLOCK`/`DESTROY_BLOCK`.
> 
> This replacement involves two aspects:
> 
> 1. Renaming: The term "volume" is overloaded and created name conflicts
>in mesos. This renaming would lead to cleaner names in ACL and other
>places, and is more consistent with CSI.
> 
> 2. Removing PATH support. PATH disks are used to split a disk resource
>into smaller chunks for concurrent independent uses. However, this
>concept does not quite fit CSI well.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 463e6adcdf14115b2f21270bb1fd9c45e1b67cc3 
>   include/mesos/v1/mesos.proto 8eaad9c4b2a9cdd527922a004b0f7dd0dc58a7f1 
> 
> 
> Diff: https://reviews.apache.org/r/67779/diff/2/
> 
> 
> Testing
> ---
> 
> Test done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67779: Introduced `CREATE_DISK` and `DESTROY_DISK` operations.

[Chun-Hung Hsiao]

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

(Updated July 10, 2018, 3:10 a.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, Greg Mann, James 
DeFelice, and Jie Yu.


Changes
---

Removed WIP.


Summary (updated)
-

Introduced `CREATE_DISK` and `DESTROY_DISK` operations.


Repository: mesos


Description
---

This patch introduces `CREATE_DISK` and `DESTROY_DISK` operations for
conversion between RAW disk resaurces and MOUNT/BLOCK disk resources to
replace `CREATE_VOLUME`/`DESTROY_VOLUME`/`CREATE_BLOCK`/`DESTROY_BLOCK`.

This replacement involves two aspects:

1. Renaming: The term "volume" is overloaded and created name conflicts
   in mesos. This renaming would lead to cleaner names in ACL and other
   places, and is more consistent with CSI.

2. Removing PATH support. PATH disks are used to split a disk resource
   into smaller chunks for concurrent independent uses. However, this
   concept does not quite fit CSI well.


Diffs (updated)
-

  include/mesos/mesos.proto 463e6adcdf14115b2f21270bb1fd9c45e1b67cc3 
  include/mesos/v1/mesos.proto 8eaad9c4b2a9cdd527922a004b0f7dd0dc58a7f1 


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

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


Testing (updated)
---

Test done later in chain.


Thanks,

Chun-Hung Hsiao