Review Request 70622: Added a unit test to verify if SLRP allows changes in volume context.

2019-05-09 Thread Chun-Hung Hsiao 

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

Review request for mesos and Benjamin Bannier.


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


Repository: mesos


Description
---

Added a unit test to verify if SLRP allows changes in volume context.


Diffs
-

  src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598 
  src/tests/storage_local_resource_provider_tests.cpp 
ecd7aeef1cb3d1a5b4b3419dfd912d41a8c6 


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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70622: Added a unit test to verify if SLRP allows changes in volume context.

2019-05-10 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [70620, 70621, 70622]

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

- Mesos Reviewbot


On May 10, 2019, 1:20 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70622/
> ---
> 
> (Updated May 10, 2019, 1:20 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9395
> https://issues.apache.org/jira/browse/MESOS-9395
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a unit test to verify if SLRP allows changes in volume context.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ecd7aeef1cb3d1a5b4b3419dfd912d41a8c6 
> 
> 
> Diff: https://reviews.apache.org/r/70622/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70622: Added a unit test to verify if SLRP allows changes in volume context.

2019-05-13 Thread Benjamin Bannier 

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


Fix it, then Ship it!




Ship It!


src/examples/test_csi_plugin.cpp
Lines 181-187 (original), 188-195 (patched)


This is a pretty fragile interface with parameters of identical or very 
similar types being passed close to each other in multiple spots 
(`Option`, `string`, `string`, and `hashmap`, 
`hashmap`). Let's try to e.g., define dedicated and distinct 
types if we build similar interfaces in the future.



src/examples/test_csi_plugin.cpp
Lines 2010-2021 (patched)


Any reason we don't error check first and possibly return early, and only 
then mutate `volumeMetadata`?

For you extensible solution we could got with e.g., 

if (values.size() != 1) {
error = "Metadata keys must be unique";
}

if (error.isSome()) {
   
}

volumeMetadata.put(prop, values[0]);


This could be simplified for the current validation.



src/tests/storage_local_resource_provider_tests.cpp
Lines 1269 (patched)


Do we actually care about this given that we wait for offers from the agent 
below anyway? I'd remove this to focus the test more.


- Benjamin Bannier


On May 10, 2019, 3:20 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70622/
> ---
> 
> (Updated May 10, 2019, 3:20 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9395
> https://issues.apache.org/jira/browse/MESOS-9395
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a unit test to verify if SLRP allows changes in volume context.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ecd7aeef1cb3d1a5b4b3419dfd912d41a8c6 
> 
> 
> Diff: https://reviews.apache.org/r/70622/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70622: Added a unit test to verify if SLRP allows changes in volume context.

2019-05-21 Thread Mesos Reviewbot 

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



Bad review!

Reviews applied: [70622, 70621, 70620]

Error:
2019-05-22 03:06:00 URL:https://reviews.apache.org/r/70622/diff/raw/ 
[13556/13556] -> "70622.patch" [1]
error: patch failed: src/examples/test_csi_plugin.cpp:217
error: src/examples/test_csi_plugin.cpp: patch does not apply

- Mesos Reviewbot


On May 9, 2019, 6:20 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70622/
> ---
> 
> (Updated May 9, 2019, 6:20 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9395
> https://issues.apache.org/jira/browse/MESOS-9395
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a unit test to verify if SLRP allows changes in volume context.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ecd7aeef1cb3d1a5b4b3419dfd912d41a8c6 
> 
> 
> Diff: https://reviews.apache.org/r/70622/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70622: Added a unit test to verify if SLRP allows changes in volume context.

2019-05-21 Thread Chun-Hung Hsiao 

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

(Updated May 22, 2019, 3:15 a.m.)


Review request for mesos and Benjamin Bannier.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

Added a unit test to verify if SLRP allows changes in volume context.


Diffs (updated)
-

  src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598 
  src/tests/storage_local_resource_provider_tests.cpp 
ecd7aeef1cb3d1a5b4b3419dfd912d41a8c6 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao