Re: Review Request 65032: Added a SLRP unit test for agent reboot.

2018-02-06 Thread Jie Yu 

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


Ship it!




Ship It!

- Jie Yu


On Feb. 2, 2018, 5:07 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65032/
> ---
> 
> (Updated Feb. 2, 2018, 5:07 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8415
> https://issues.apache.org/jira/browse/MESOS-8415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a SLRP unit test for agent reboot.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ce241617ee555e5a67e552dd30b0e7afee161386 
> 
> 
> Diff: https://reviews.apache.org/r/65032/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65032: Added a SLRP unit test for agent reboot.

2018-02-01 Thread Chun-Hung Hsiao 

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

(Updated Feb. 2, 2018, 5:07 a.m.)


Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.


Changes
---

Addressed Greg's comments.


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


Repository: mesos


Description
---

Added a SLRP unit test for agent reboot.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
85eaef877cb3d62aed0eb2bd5adfba8007c3ee76 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65032: Added a SLRP unit test for agent reboot.

2018-02-01 Thread Chun-Hung Hsiao 


> On Jan. 18, 2018, 6:49 p.m., Greg Mann wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 1749 (patched)
> > 
> >
> > I'm curious why you place all expectations together in some tests, and 
> > spread them throughout the test here and elsewhere?
> 
> Chun-Hung Hsiao wrote:
> It's just the first one or two where I set all expectations together, 
> then in the succeeding tests I feel the logic might be easier to 
> follow/modify if they're set as the test progresses. Also in some tests I 
> want to wait for the resources obtained in previous offers. I still keep the 
> declarations and comments together as they provide a detailed description of 
> the scenario we're testing. I could use a follow-up patch to rewrite the 
> first few tests to make it consistent.

Addressed in the next patch in the chain.


- Chun-Hung


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


On Jan. 10, 2018, 2:36 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65032/
> ---
> 
> (Updated Jan. 10, 2018, 2:36 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8415
> https://issues.apache.org/jira/browse/MESOS-8415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a SLRP unit test for agent reboot.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/65032/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65032: Added a SLRP unit test for agent reboot.

2018-01-25 Thread Chun-Hung Hsiao 


> On Jan. 18, 2018, 6:49 p.m., Greg Mann wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 1749 (patched)
> > 
> >
> > I'm curious why you place all expectations together in some tests, and 
> > spread them throughout the test here and elsewhere?

It's just the first one or two where I set all expectations together, then in 
the succeeding tests I feel the logic might be easier to follow/modify if 
they're set as the test progresses. Also in some tests I want to wait for the 
resources obtained in previous offers. I still keep the declarations and 
comments together as they provide a detailed description of the scenario we're 
testing. I could use a follow-up patch to rewrite the first few tests to make 
it consistent.


- Chun-Hung


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


On Jan. 10, 2018, 2:36 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65032/
> ---
> 
> (Updated Jan. 10, 2018, 2:36 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8415
> https://issues.apache.org/jira/browse/MESOS-8415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a SLRP unit test for agent reboot.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/65032/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65032: Added a SLRP unit test for agent reboot.

2018-01-18 Thread Greg Mann 

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




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


s/agentFlags/slaveFlags/g

Or, let's just follow up at the end of the chain with a patch. I'm fine 
either way.



src/tests/storage_local_resource_provider_tests.cpp
Lines 1703-1711 (patched)


Ditto here as in parent review - I think we can make this more concise?



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


I'm curious why you place all expectations together in some tests, and 
spread them throughout the test here and elsewhere?



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


s/agent/slave/



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


s/volume/volumes/



src/tests/storage_local_resource_provider_tests.cpp
Lines 1780-1781 (patched)


Do we need the `.Times(Atleast(1))` here? I think we can rely on a single 
offer being rescinded when the agent is terminated?



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


Could we move the registering of this expectation out of this scope, and 
directly above the `StartSlave()` call below?


- Greg Mann


On Jan. 10, 2018, 2:36 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65032/
> ---
> 
> (Updated Jan. 10, 2018, 2:36 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8415
> https://issues.apache.org/jira/browse/MESOS-8415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a SLRP unit test for agent reboot.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/65032/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65032: Added a SLRP unit test for agent reboot.

2018-01-09 Thread Chun-Hung Hsiao 

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

(Updated Jan. 10, 2018, 2:36 a.m.)


Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.


Changes
---

Speeded up the test.


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


Repository: mesos


Description
---

Added a SLRP unit test for agent reboot.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
bbfe95e9818f25fdd5405db3ad2fe355e023f743 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65032: Added a SLRP unit test for agent reboot.

2018-01-08 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 65032 was successfully built and tested.

Reviews applied: `['65029', '65021', '65022', '65023', '65024', '65025', 
'65026', '64992', '64994', '64998', '65000', '65032']`

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

- Mesos Reviewbot Windows


On Jan. 8, 2018, 8:41 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65032/
> ---
> 
> (Updated Jan. 8, 2018, 8:41 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8415
> https://issues.apache.org/jira/browse/MESOS-8415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a SLRP unit test for agent reboot.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbfe95e9818f25fdd5405db3ad2fe355e023f743 
> 
> 
> Diff: https://reviews.apache.org/r/65032/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 65032: Added a SLRP unit test for agent reboot.

2018-01-08 Thread Chun-Hung Hsiao 

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

Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.


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


Repository: mesos


Description
---

Added a SLRP unit test for agent reboot.


Diffs
-

  src/tests/storage_local_resource_provider_tests.cpp 
bbfe95e9818f25fdd5405db3ad2fe355e023f743 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao