Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





src/resource_provider/storage/provider.cpp
Lines 2869-2871 (patched)


```
case Offer::Operation::GROW_VOLUME:
case Offer::Operation::SHRINK_VOLUME: {
  // TODO(zhitao): These operations are currently not supported for
  // resource providers, and should have been validated by the master.
  UNIMPLEMENTED;
}
```
And also include `stout/unimplemented.hpp`.
Or you can leave my id (`chhsiao`) here instead.


- Chun-Hung Hsiao


On April 27, 2018, 7:23 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated April 27, 2018, 7:23 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5bc4a80ad5278d20feced73dce755519515de505 
>   include/mesos/v1/mesos.proto 5a4e733250831fa5fe86544aeb98dbc0a4f5afa6 
>   src/common/protobuf_utils.cpp 78bffd8595f0e9f34e981548d8136ff94160573b 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 
>   src/resource_provider/storage/provider.cpp 
> 8ca2d3a98858940e6e027becefb53c2f00b4ae43 
>   src/tests/mesos.hpp 756a52189ea3336b203ea1d606e2ba17762d57aa 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
>   src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/12/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

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

(Updated April 27, 2018, 12:23 p.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Fix build when grpc is enabled.


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


Repository: mesos


Description
---

Added offer operation to grow and shrink persistent volumes.


Diffs (updated)
-

  include/mesos/mesos.proto 5bc4a80ad5278d20feced73dce755519515de505 
  include/mesos/v1/mesos.proto 5a4e733250831fa5fe86544aeb98dbc0a4f5afa6 
  src/common/protobuf_utils.cpp 78bffd8595f0e9f34e981548d8136ff94160573b 
  src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
  src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 
  src/resource_provider/storage/provider.cpp 
8ca2d3a98858940e6e027becefb53c2f00b4ae43 
  src/tests/mesos.hpp 756a52189ea3336b203ea1d606e2ba17762d57aa 
  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 
  src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 


Diff: https://reviews.apache.org/r/66049/diff/12/

Changes: https://reviews.apache.org/r/66049/diff/11-12/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

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

(Updated April 20, 2018, 11:38 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added offer operation to grow and shrink persistent volumes.


Diffs (updated)
-

  include/mesos/mesos.proto 9e24d3ea46edc21572e9226e2e76c7d55618db24 
  include/mesos/v1/mesos.proto 0f3fd8a2608b5edabc21f5fe5df9b70fc0fa8dc2 
  src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
  src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
  src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
  src/tests/mesos.hpp 7356523a2a37858bbe866527425f35502ff5ad82 
  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 
  src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 


Diff: https://reviews.apache.org/r/66049/diff/10/

Changes: https://reviews.apache.org/r/66049/diff/9-10/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On April 19, 2018, 4:28 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated April 19, 2018, 4:28 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
>   src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
>   src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
>   src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

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

(Updated April 19, 2018, 9:28 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Add comment on scalar precision.


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


Repository: mesos


Description
---

Added offer operation to grow and shrink persistent volumes.


Diffs (updated)
-

  include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
  include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
  src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
  src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
  src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
  src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 
  src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 


Diff: https://reviews.apache.org/r/66049/diff/9/

Changes: https://reviews.apache.org/r/66049/diff/8-9/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

> On April 12, 2018, 5:27 p.m., Greg Mann wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > 
> >
> > Hmm I wonder if we should just use a `double` here? Does the 
> > `Value.Scalar` type provide some benefit?
> 
> Chun-Hung Hsiao wrote:
> From the API perspective, `double` is easier to use, but `Value.Scalar` 
> is more consistent with how we represesnt a disk resource.
> 
> From the implementation perspective, I prefer `Value.Scalar` because it's 
> arithmetic operators will do fixed-point conversions for me so I won't forget 
> that.
> 
> Greg Mann wrote:
> My recommendation would be to prioritize easy-of-use in the API over 
> ease-of-implementation when possible :)
> 
> In the implementation, as long as we toss the provided double into a 
> `Scalar::Value` before we do anything with it, I think we could get the same 
> benefit?
> 
> Zhitao Li wrote:
> I feel that `Scalar` is actually more consistent than `double` here. I do 
> not see either API is much easier to use.
> 
> Greg Mann wrote:
> Why is `Scalar` more consistent? I think that `double` would be simpler 
> because it eliminates the nested `value` field, which is not intuitive.
> 
> Is there any reason to make this a `Scalar` other than the fact that it 
> makes our implementation simpler? Do we have other APIs which accept a 
> `Value.Scalar` for similar purposes?
> 
> Chun-Hung Hsiao wrote:
> Actually my point is not about the ease of implementation, but to prevent 
> a careless developer like me ;) to forget to do the fixed-point conversion 
> when doing resource arithmetic. But still, this is a pure implementation 
> concern, not an API concern.

Capturing offline conversation, we will keep `Scalar` but add a comment asking 
reader to see Scalar's comment about precision.


- Zhitao


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


On April 13, 2018, 10:20 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated April 13, 2018, 10:20 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
>   src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
>   src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
>   src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Chun-Hung Hsiao]

> On April 13, 2018, 12:27 a.m., Greg Mann wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > 
> >
> > Hmm I wonder if we should just use a `double` here? Does the 
> > `Value.Scalar` type provide some benefit?
> 
> Chun-Hung Hsiao wrote:
> From the API perspective, `double` is easier to use, but `Value.Scalar` 
> is more consistent with how we represesnt a disk resource.
> 
> From the implementation perspective, I prefer `Value.Scalar` because it's 
> arithmetic operators will do fixed-point conversions for me so I won't forget 
> that.
> 
> Greg Mann wrote:
> My recommendation would be to prioritize easy-of-use in the API over 
> ease-of-implementation when possible :)
> 
> In the implementation, as long as we toss the provided double into a 
> `Scalar::Value` before we do anything with it, I think we could get the same 
> benefit?
> 
> Zhitao Li wrote:
> I feel that `Scalar` is actually more consistent than `double` here. I do 
> not see either API is much easier to use.
> 
> Greg Mann wrote:
> Why is `Scalar` more consistent? I think that `double` would be simpler 
> because it eliminates the nested `value` field, which is not intuitive.
> 
> Is there any reason to make this a `Scalar` other than the fact that it 
> makes our implementation simpler? Do we have other APIs which accept a 
> `Value.Scalar` for similar purposes?

Actually my point is not about the ease of implementation, but to prevent a 
careless developer like me ;) to forget to do the fixed-point conversion when 
doing resource arithmetic. But still, this is a pure implementation concern, 
not an API concern.


- Chun-Hung


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


On April 13, 2018, 5:20 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated April 13, 2018, 5:20 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
>   src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
>   src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
>   src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Greg Mann]

> On April 13, 2018, 12:27 a.m., Greg Mann wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > 
> >
> > Hmm I wonder if we should just use a `double` here? Does the 
> > `Value.Scalar` type provide some benefit?
> 
> Chun-Hung Hsiao wrote:
> From the API perspective, `double` is easier to use, but `Value.Scalar` 
> is more consistent with how we represesnt a disk resource.
> 
> From the implementation perspective, I prefer `Value.Scalar` because it's 
> arithmetic operators will do fixed-point conversions for me so I won't forget 
> that.
> 
> Greg Mann wrote:
> My recommendation would be to prioritize easy-of-use in the API over 
> ease-of-implementation when possible :)
> 
> In the implementation, as long as we toss the provided double into a 
> `Scalar::Value` before we do anything with it, I think we could get the same 
> benefit?
> 
> Zhitao Li wrote:
> I feel that `Scalar` is actually more consistent than `double` here. I do 
> not see either API is much easier to use.

Why is `Scalar` more consistent? I think that `double` would be simpler because 
it eliminates the nested `value` field, which is not intuitive.

Is there any reason to make this a `Scalar` other than the fact that it makes 
our implementation simpler? Do we have other APIs which accept a `Value.Scalar` 
for similar purposes?


- Greg


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


On April 13, 2018, 5:20 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated April 13, 2018, 5:20 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
>   src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
>   src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
>   src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

> On April 12, 2018, 5:27 p.m., Greg Mann wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > 
> >
> > Hmm I wonder if we should just use a `double` here? Does the 
> > `Value.Scalar` type provide some benefit?
> 
> Chun-Hung Hsiao wrote:
> From the API perspective, `double` is easier to use, but `Value.Scalar` 
> is more consistent with how we represesnt a disk resource.
> 
> From the implementation perspective, I prefer `Value.Scalar` because it's 
> arithmetic operators will do fixed-point conversions for me so I won't forget 
> that.
> 
> Greg Mann wrote:
> My recommendation would be to prioritize easy-of-use in the API over 
> ease-of-implementation when possible :)
> 
> In the implementation, as long as we toss the provided double into a 
> `Scalar::Value` before we do anything with it, I think we could get the same 
> benefit?

I feel that `Scalar` is actually more consistent than `double` here. I do not 
see either API is much easier to use.


- Zhitao


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


On April 13, 2018, 10:20 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated April 13, 2018, 10:20 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
>   src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
>   src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
>   src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

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

(Updated April 13, 2018, 10:20 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Remove unnecessary whiteline.


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


Repository: mesos


Description
---

Added offer operation to grow and shrink persistent volumes.


Diffs (updated)
-

  include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
  include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
  src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
  src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
  src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
  src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 
  src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 


Diff: https://reviews.apache.org/r/66049/diff/8/

Changes: https://reviews.apache.org/r/66049/diff/7-8/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

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

(Updated April 13, 2018, 10:15 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Add `UNIMPLEMENTED` annotations so this compiles.


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


Repository: mesos


Description
---

Added offer operation to grow and shrink persistent volumes.


Diffs (updated)
-

  include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
  include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
  src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
  src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
  src/master/master.cpp 767ad8cfe142b47ef07172bcb2a4fb49fc3e833a 
  src/tests/mesos.hpp 6f4e0c5567b99891f6d0eceb7e6917d25083db0e 
  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 
  src/tests/reservation_tests.cpp 5570df2e0b208512d0a0a3079a180e1acfe08f3d 


Diff: https://reviews.apache.org/r/66049/diff/7/

Changes: https://reviews.apache.org/r/66049/diff/6-7/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Greg Mann]

> On April 13, 2018, 12:27 a.m., Greg Mann wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > 
> >
> > Hmm I wonder if we should just use a `double` here? Does the 
> > `Value.Scalar` type provide some benefit?
> 
> Chun-Hung Hsiao wrote:
> From the API perspective, `double` is easier to use, but `Value.Scalar` 
> is more consistent with how we represesnt a disk resource.
> 
> From the implementation perspective, I prefer `Value.Scalar` because it's 
> arithmetic operators will do fixed-point conversions for me so I won't forget 
> that.

My recommendation would be to prioritize easy-of-use in the API over 
ease-of-implementation when possible :)

In the implementation, as long as we toss the provided double into a 
`Scalar::Value` before we do anything with it, I think we could get the same 
benefit?


- Greg


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


On March 28, 2018, 6:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Chun-Hung Hsiao]

> On April 13, 2018, 12:27 a.m., Greg Mann wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > 
> >
> > Hmm I wonder if we should just use a `double` here? Does the 
> > `Value.Scalar` type provide some benefit?

>From the API perspective, `double` is easier to use, but `Value.Scalar` is 
>more consistent with how we represesnt a disk resource.

>From the implementation perspective, I prefer `Value.Scalar` because it's 
>arithmetic operators will do fixed-point conversions for me so I won't forget 
>that.


- Chun-Hung


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


On March 28, 2018, 6:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Chun-Hung Hsiao]

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




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


Let's remove the two blank lines. Ditto in `v1/mesos.proto`.


- Chun-Hung Hsiao


On March 28, 2018, 6:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Greg Mann]

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




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


Hmm I wonder if we should just use a `double` here? Does the `Value.Scalar` 
type provide some benefit?


- Greg Mann


On March 28, 2018, 6:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

> On March 29, 2018, 5:07 a.m., Benjamin Bannier wrote:
> > include/mesos/mesos.proto
> > Lines 1920-1921 (patched)
> > 
> >
> > These enum values are not handled in a number of switch statements in 
> > the code so that this patch cannot be used in isolation (fatal compiler 
> > warnings). It might be enough to enumerate the new cases explicitly and 
> > mark them `UNIMPLEMENTED`.
> > 
> > Similar for v1 enum below.
> 
> Zhitao Li wrote:
> My plan was to commit all patches leading to proper full implementation 
> in one shot so code on master is still compilable (which seems previous norm 
> to me?)
> 
> If you think marking them as `UNIMPLEMENTED` then gradually implement 
> each in later patches is better, I can happily do that.
> 
> Greg Mann wrote:
> We generally try to make each patch self-contained, so that the codebase 
> will still compile and tests will still pass if that patch were applied 
> alone. We do sometimes break from this when it's very difficult to 
> accomplish. I think that Benjamin's comment here is valid; I'll leave it up 
> to you what you want to do with this patch, but in the future, try to make 
> patches self-contained when possible.

That's fair. I like your proposal to keep patches self-contained if possible 
(just weren't clear on this practice).

I'll add `UNIMPLEMENTED();` to broken enum checks in this patch.


- Zhitao


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


On March 28, 2018, 11:24 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 11:24 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Chun-Hung Hsiao]

> On March 29, 2018, 12:07 p.m., Benjamin Bannier wrote:
> > include/mesos/mesos.proto
> > Lines 1920-1921 (patched)
> > 
> >
> > These enum values are not handled in a number of switch statements in 
> > the code so that this patch cannot be used in isolation (fatal compiler 
> > warnings). It might be enough to enumerate the new cases explicitly and 
> > mark them `UNIMPLEMENTED`.
> > 
> > Similar for v1 enum below.
> 
> Zhitao Li wrote:
> My plan was to commit all patches leading to proper full implementation 
> in one shot so code on master is still compilable (which seems previous norm 
> to me?)
> 
> If you think marking them as `UNIMPLEMENTED` then gradually implement 
> each in later patches is better, I can happily do that.
> 
> Greg Mann wrote:
> We generally try to make each patch self-contained, so that the codebase 
> will still compile and tests will still pass if that patch were applied 
> alone. We do sometimes break from this when it's very difficult to 
> accomplish. I think that Benjamin's comment here is valid; I'll leave it up 
> to you what you want to do with this patch, but in the future, try to make 
> patches self-contained when possible.
> 
> Zhitao Li wrote:
> That's fair. I like your proposal to keep patches self-contained if 
> possible (just weren't clear on this practice).
> 
> I'll add `UNIMPLEMENTED();` to broken enum checks in this patch.

If this patch cannot be built standalone, please leave a note in the "Testing 
Done" text box to remind us to push this commit along with the following 
patches :)


- Chun-Hung


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


On March 28, 2018, 6:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Greg Mann]

> On March 29, 2018, 12:07 p.m., Benjamin Bannier wrote:
> > include/mesos/mesos.proto
> > Lines 1920-1921 (patched)
> > 
> >
> > These enum values are not handled in a number of switch statements in 
> > the code so that this patch cannot be used in isolation (fatal compiler 
> > warnings). It might be enough to enumerate the new cases explicitly and 
> > mark them `UNIMPLEMENTED`.
> > 
> > Similar for v1 enum below.
> 
> Zhitao Li wrote:
> My plan was to commit all patches leading to proper full implementation 
> in one shot so code on master is still compilable (which seems previous norm 
> to me?)
> 
> If you think marking them as `UNIMPLEMENTED` then gradually implement 
> each in later patches is better, I can happily do that.

We generally try to make each patch self-contained, so that the codebase will 
still compile and tests will still pass if that patch were applied alone. We do 
sometimes break from this when it's very difficult to accomplish. I think that 
Benjamin's comment here is valid; I'll leave it up to you what you want to do 
with this patch, but in the future, try to make patches self-contained when 
possible.


- Greg


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


On March 28, 2018, 6:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

> On March 29, 2018, 5:07 a.m., Benjamin Bannier wrote:
> > include/mesos/mesos.proto
> > Lines 1920-1921 (patched)
> > 
> >
> > These enum values are not handled in a number of switch statements in 
> > the code so that this patch cannot be used in isolation (fatal compiler 
> > warnings). It might be enough to enumerate the new cases explicitly and 
> > mark them `UNIMPLEMENTED`.
> > 
> > Similar for v1 enum below.

My plan was to commit all patches leading to proper full implementation in one 
shot so code on master is still compilable (which seems previous norm to me?)

If you think marking them as `UNIMPLEMENTED` then gradually implement each in 
later patches is better, I can happily do that.


- Zhitao


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


On March 28, 2018, 11:24 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 11:24 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Benjamin Bannier]

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




include/mesos/mesos.proto
Lines 1920-1921 (patched)


These enum values are not handled in a number of switch statements in the 
code so that this patch cannot be used in isolation (fatal compiler warnings). 
It might be enough to enumerate the new cases explicitly and mark them 
`UNIMPLEMENTED`.

Similar for v1 enum below.


- Benjamin Bannier


On March 28, 2018, 8:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 8:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 28, 2018, 6:24 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 28, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

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

(Updated March 28, 2018, 11:24 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


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


Repository: mesos


Description
---

Added offer operation to grow and shrink persistent volumes.


Diffs (updated)
-

  include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
  include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Greg Mann]

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




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


s/size/the size/


- Greg Mann


On March 26, 2018, 4:53 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 26, 2018, 4:53 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

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

(Updated March 22, 2018, 3:36 p.m.)


Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.


Changes
---

Updated comment.


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


Repository: mesos


Description
---

Added offer operation to grow and shrink persistent volumes.


Diffs (updated)
-

  include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
  include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





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


How about "Grow a volume by an additional disk resource"? Ditto below.



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


"Shrink a volume by size specified in the `subtract` field" here and below.


- Chun-Hung Hsiao


On March 21, 2018, 1:31 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 21, 2018, 1:31 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9 
>   include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

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

(Updated March 20, 2018, 6:31 p.m.)


Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.


Changes
---

Updated API.


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


Repository: mesos


Description
---

Added offer operation to grow and shrink persistent volumes.


Diffs (updated)
-

  include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9 
  include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Jie Yu]

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




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


I'd suggest the following:
```
message GrowVolume {
  required Resource volume = 1;
  required Resource addition = 2;
}
```

Using `Resource` for `addition` make it clear that you need an offer 
containing the resources to be able to grow an volume. You cannot bindly grown 
a volume. The master will validate that `addition` is actually allocated to the 
framework.

The master will also need to validate that the `addition` Resource is 
compatible with `volume` in the operation.
1) If `volume` is a `ROOT` disk, `addition` has to be a `ROOT` disk too.
2) If `volume` is a `PATH` disk, the `addition` can either be a `PATH` disk 
with the same `Source`, or a `RAW` disk from the same resource provider and the 
same profile.
3) If `volume` is a `MOUNT` disk, the `addition` should be a `RAW` disk 
from the same resource provider and the same profile.

Given that, using a `Resource` for `addition` is more appropriate and 
future proof.


- Jie Yu


On March 16, 2018, 5:54 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 16, 2018, 5:54 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9 
>   include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

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

(Updated March 16, 2018, 10:54 a.m.)


Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.


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


Repository: mesos


Description
---

Added offer operation to grow and shrink persistent volumes.


Diffs (updated)
-

  include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9 
  include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Chun-Hung Hsiao]

> On March 15, 2018, 7:10 p.m., Chun-Hung Hsiao wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > 
> >
> > I'm thinking that, instead of asking the framework to craft the freed 
> > disk, we could leave it to the agent/RP so they have the freedom to 
> > transform the freed disk to an appropriate type of disk resource (although 
> > for now it will be the same as the original volume except in size and 
> > persistence). So how about having a `Scalar target` instead, and we can 
> > implement it through `Resources::shrink()`?
> 
> Zhitao Li wrote:
> I suggest we do not look at how `Resources` class implements various 
> helpers when designing public API, but rather think about how to make the API 
> easy, straightforward and consistent for the users (here it would be 
> framework authors).
> 
> Initially, I chose this format because it is more symetrical to 
> `RESERVE`/`UNRESERVE` and could be chained together in one operation. 
> However, after the decision to make new API non-chainable (and 
> non-speculative), that argument seems weaker now.
> 
> If we go with `target` format, I'd rather make the target as a full 
> `Resource` object and the API will look like:
> 
> ```
> message SHRINK_VOLUME {
>   required Resource original = 1; // original volume resource
>   required Resource target = 2;  // target volume resource
> }
> ```
> 
> Framework author can just make a copy of `original` and change the scalar 
> quantity.
> 
> Eventually, we might be able to mark `original` optional or drop it in 
> the API.
> 
> What do you think?

Yeah you're right about not making API decision based on utility functions.

Dropping `original` doesn't sound a good idea to me for the following reasons:
1. Consistency-wise, it would be different from other non-speculative calls 
such as `CREATE_VOLUME` or `DESTROY_VOLUME` (NOTE: I'm not talking about the 
`CREATE_VOLUMES` or `DESTROY_VOLUMES` operator calls), where we specify the 
source resource and let the operation performer to craft the converted.results.
2. Implementation-wise, the master and agent still need to find the original 
resource to construct and apply the resource conversion. If that doesn't come 
from the request, we need to do an extra search to find the source, which I 
don't feel necessary.

For `target` scalar vs resource, we could think about what are the restrictions 
we'd like to enforce. If we want to make sure that the resulting shrunk 
resource must look exactly the same and have no other metadata change, then I'm 
fine with making it a `Reaource`. If we want to have minimal restrictions and 
give the operation performer some freedom to modify the resoure (for example, 
make `taeget` a reference size or upper limit and thus the agent can shrink the 
volume as small as possible), then I feel a scalar looks better. FYI, in 
`CREATE_VOLUME`, we specify a `source` and a target type (`MOUNT` or `PATH`) 
only instead of the resulting resource, because the reaource provider will fill 
in extra information such as `id` and `metadata`.


- Chun-Hung


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


On March 13, 2018, 10:48 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 13, 2018, 10:48 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9 
>   include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

> On March 15, 2018, 12:10 p.m., Chun-Hung Hsiao wrote:
> > include/mesos/mesos.proto
> > Lines 1920-1921 (patched)
> > 
> >
> > As we discussed, let's do `s/PERSISTENT_//`, as well as 
> > `s/Persistent//` and `s/persistent_//` below.

Will do.


> On March 15, 2018, 12:10 p.m., Chun-Hung Hsiao wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > 
> >
> > I'm thinking that, instead of asking the framework to craft the freed 
> > disk, we could leave it to the agent/RP so they have the freedom to 
> > transform the freed disk to an appropriate type of disk resource (although 
> > for now it will be the same as the original volume except in size and 
> > persistence). So how about having a `Scalar target` instead, and we can 
> > implement it through `Resources::shrink()`?

I suggest we do not look at how `Resources` class implements various helpers 
when designing public API, but rather think about how to make the API easy, 
straightforward and consistent for the users (here it would be framework 
authors).

Initially, I chose this format because it is more symetrical to 
`RESERVE`/`UNRESERVE` and could be chained together in one operation. However, 
after the decision to make new API non-chainable (and non-speculative), that 
argument seems weaker now.

If we go with `target` format, I'd rather make the target as a full `Resource` 
object and the API will look like:

```
message SHRINK_VOLUME {
  required Resource original = 1; // original volume resource
  required Resource target = 2;  // target volume resource
}
```

Framework author can just make a copy of `original` and change the scalar 
quantity.

Eventually, we might be able to mark `original` optional or drop it in the API.

What do you think?


- Zhitao


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


On March 13, 2018, 3:48 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 13, 2018, 3:48 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9 
>   include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Chun-Hung Hsiao]

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




include/mesos/mesos.proto
Lines 1920-1921 (patched)


As we discussed, let's do `s/PERSISTENT_//`, as well as `s/Persistent//` 
and `s/persistent_//` below.



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


I'm thinking that, instead of asking the framework to craft the freed disk, 
we could leave it to the agent/RP so they have the freedom to transform the 
freed disk to an appropriate type of disk resource (although for now it will be 
the same as the original volume except in size and persistence). So how about 
having a `Scalar target` instead, and we can implement it through 
`Resources::shrink()`?


- Chun-Hung Hsiao


On March 13, 2018, 10:48 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 13, 2018, 10:48 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9 
>   include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>