Re: Review Request 42861: Ensured two Mount Disk resources do not have the same root path.

2017-03-01 Thread Joris Van Remoortere 

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



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On Sept. 12, 2016, 5:29 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42861/
> ---
> 
> (Updated Sept. 12, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-4521
> https://issues.apache.org/jira/browse/MESOS-4521
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured two Mount Disk resources do not have the same root path.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> 
> Diff: https://reviews.apache.org/r/42861/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42861: Ensured two Mount Disk resources do not have the same root path.

2016-10-03 Thread Greg Mann 

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




src/slave/slave.cpp 


Why are you getting rid of this logic?


- Greg Mann


On Sept. 12, 2016, 5:29 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42861/
> ---
> 
> (Updated Sept. 12, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-4521
> https://issues.apache.org/jira/browse/MESOS-4521
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured two Mount Disk resources do not have the same root path.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/42861/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42861: Ensured two Mount Disk resources do not have the same root path.

2016-10-03 Thread Greg Mann 


> On Sept. 6, 2016, 10:38 p.m., Greg Mann wrote:
> > Thanks Haosdent! Looks good. Could you also add a test for this?
> 
> haosdent huang wrote:
> Hi, @greggomann Thanks a lot for your reviews and helps! For test cases, 
> I think we have covered it in https://reviews.apache.org/r/42860/. So I think 
> not need to add it here.

We could also add an integration test which actually spins up an agent and 
checks that it will exit if it's given mount volumes with overlapping paths. 
However, in this case the unit tests may be good enough.


- Greg


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


On Sept. 12, 2016, 5:29 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42861/
> ---
> 
> (Updated Sept. 12, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-4521
> https://issues.apache.org/jira/browse/MESOS-4521
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured two Mount Disk resources do not have the same root path.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/42861/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42861: Ensured two Mount Disk resources do not have the same root path.

2016-09-20 Thread haosdent huang 


> On Sept. 7, 2016, 7:21 p.m., Joris Van Remoortere wrote:
> > src/slave/slave.cpp, lines 559-561
> > 
> >
> > We should improve the error information here. Why can't we identify 
> > which original paths that were provided are overlapping?
> > If we need to improve a library function we can add a TODO here and 
> > follow up with a patch.
> 
> haosdent huang wrote:
> Hi, @jvanremoortere, do you mean we should check 
> `path::overlapping(source.mount().root())` without call `realpath`?
> 
> Joris Van Remoortere wrote:
> Hey @haosdent,
> No, I think using realpath is correct.
> I mean that a user currently won't have a good idea about *which* paths 
> are overlapping.
> Should `path::overlapping()` return an `Option` or some 
> other way to provide the list of actually overlapping paths so that we can 
> provide a more meaningful error message?

Make sense! Let me update.


- haosdent


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


On Sept. 12, 2016, 5:29 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42861/
> ---
> 
> (Updated Sept. 12, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-4521
> https://issues.apache.org/jira/browse/MESOS-4521
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured two Mount Disk resources do not have the same root path.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/42861/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42861: Ensured two Mount Disk resources do not have the same root path.

2016-09-20 Thread Joris Van Remoortere 


> On Sept. 7, 2016, 7:21 p.m., Joris Van Remoortere wrote:
> > src/slave/slave.cpp, lines 559-561
> > 
> >
> > We should improve the error information here. Why can't we identify 
> > which original paths that were provided are overlapping?
> > If we need to improve a library function we can add a TODO here and 
> > follow up with a patch.
> 
> haosdent huang wrote:
> Hi, @jvanremoortere, do you mean we should check 
> `path::overlapping(source.mount().root())` without call `realpath`?

Hey @haosdent,
No, I think using realpath is correct.
I mean that a user currently won't have a good idea about *which* paths are 
overlapping.
Should `path::overlapping()` return an `Option` or some other 
way to provide the list of actually overlapping paths so that we can provide a 
more meaningful error message?


- Joris


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


On Sept. 12, 2016, 5:29 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42861/
> ---
> 
> (Updated Sept. 12, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-4521
> https://issues.apache.org/jira/browse/MESOS-4521
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured two Mount Disk resources do not have the same root path.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/42861/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42861: Ensured two Mount Disk resources do not have the same root path.

2016-09-12 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [42860, 42861]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 12, 2016, 5:29 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42861/
> ---
> 
> (Updated Sept. 12, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-4521
> https://issues.apache.org/jira/browse/MESOS-4521
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured two Mount Disk resources do not have the same root path.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/42861/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42861: Ensured two Mount Disk resources do not have the same root path.

2016-09-12 Thread haosdent huang 


> On Sept. 7, 2016, 7:21 p.m., Joris Van Remoortere wrote:
> > src/slave/slave.cpp, lines 559-561
> > 
> >
> > We should improve the error information here. Why can't we identify 
> > which original paths that were provided are overlapping?
> > If we need to improve a library function we can add a TODO here and 
> > follow up with a patch.

Hi, @jvanremoortere, do you mean we should check 
`path::overlapping(source.mount().root())` without call `realpath`?


- haosdent


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


On Sept. 12, 2016, 5:29 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42861/
> ---
> 
> (Updated Sept. 12, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-4521
> https://issues.apache.org/jira/browse/MESOS-4521
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured two Mount Disk resources do not have the same root path.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/42861/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42861: Ensured two Mount Disk resources do not have the same root path.

2016-09-12 Thread haosdent huang 


> On Sept. 6, 2016, 10:38 p.m., Greg Mann wrote:
> > Thanks Haosdent! Looks good. Could you also add a test for this?

Hi, @greggomann Thanks a lot for your reviews and helps! For test cases, I 
think we have covered it in https://reviews.apache.org/r/42860/. So I think not 
need to add it here.


- haosdent


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


On Sept. 12, 2016, 5:29 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42861/
> ---
> 
> (Updated Sept. 12, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-4521
> https://issues.apache.org/jira/browse/MESOS-4521
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured two Mount Disk resources do not have the same root path.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/42861/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 42861: Ensured two Mount Disk resources do not have the same root path.

2016-09-12 Thread haosdent huang 

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

(Updated Sept. 12, 2016, 5:29 p.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and 
Neil Conway.


Changes
---

Address @greggomann's comment.


Summary (updated)
-

Ensured two Mount Disk resources do not have the same root path.


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


Repository: mesos


Description (updated)
---

Ensured two Mount Disk resources do not have the same root path.


Diffs (updated)
-

  src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 

Diff: https://reviews.apache.org/r/42861/diff/


Testing
---


Thanks,

haosdent huang