You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2012/12/19 19:03:30 UTC
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/
-----------------------------------------------------------
(Updated Dec. 19, 2012, 6:03 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Part 7 --> Part 6
Summary (updated)
-----------------
Slave Restart (Part 6): Added os::islink and substituted stat() with lstat() in os.hpp
Description
-------
Needed this for my next review.
Diffs
-----
third_party/libprocess/include/stout/os.hpp 76e5e0624af36a0021755fb4acf7f76bfb81a823
Diff: https://reviews.apache.org/r/8681/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Vinod Kone <vi...@gmail.com>.
> On Jan. 22, 2013, 4:22 a.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/stout/os.hpp, line 365
> > <https://reviews.apache.org/r/8681/diff/1/?file=241084#file241084line365>
> >
> > Okay, so if this is a link to a directory, it will now return false ... are those the semantics we want? WDPD?
actually, on second-thought, i think stat() is better here. otherwise, it would be hard to answer "is this a link to a directory/file".
> On Jan. 22, 2013, 4:22 a.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/stout/os.hpp, line 410
> > <https://reviews.apache.org/r/8681/diff/1/?file=241084#file241084line410>
> >
> > This one makes me suspicious ... what do other libraries do?
i'm not sure what libraries to look at? fwiw, 'ls' shows the mod time of the link, not the target.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/#review15558
-----------------------------------------------------------
On Dec. 19, 2012, 6:03 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8681/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2012, 6:03 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this for my next review.
>
>
> Diffs
> -----
>
> third_party/libprocess/include/stout/os.hpp 76e5e0624af36a0021755fb4acf7f76bfb81a823
>
> Diff: https://reviews.apache.org/r/8681/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/#review15558
-----------------------------------------------------------
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/8681/#comment33585>
Okay, so if this is a link to a directory, it will now return false ... are those the semantics we want? WDPD?
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/8681/#comment33586>
Ditto above.
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/8681/#comment33587>
Yeah, definitely want this one to be lstat.
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/8681/#comment33588>
This one makes me suspicious ... what do other libraries do?
- Benjamin Hindman
On Dec. 19, 2012, 6:03 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8681/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2012, 6:03 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this for my next review.
>
>
> Diffs
> -----
>
> third_party/libprocess/include/stout/os.hpp 76e5e0624af36a0021755fb4acf7f76bfb81a823
>
> Diff: https://reviews.apache.org/r/8681/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/#review17216
-----------------------------------------------------------
Ship it!
Ship It!
- Ben Mahler
On Feb. 23, 2013, 8:12 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8681/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2013, 8:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this for my next review.
>
>
> Diffs
> -----
>
> third_party/libprocess/include/stout/os.hpp 32638ee273492550491b85223cda8e7a5bde7fa5
>
> Diff: https://reviews.apache.org/r/8681/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/
-----------------------------------------------------------
(Updated March 13, 2013, 6:13 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
rebased off trunk for posterity. no need for review.
Description
-------
Needed this for my next review.
Diffs (updated)
-----
third_party/libprocess/third_party/stout/include/stout/os.hpp da14a8fb95b69684c899e04ca79a1ad8eb11b2b8
Diff: https://reviews.apache.org/r/8681/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/
-----------------------------------------------------------
(Updated Feb. 23, 2013, 8:12 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benh's
Description
-------
Needed this for my next review.
Diffs (updated)
-----
third_party/libprocess/include/stout/os.hpp 32638ee273492550491b85223cda8e7a5bde7fa5
Diff: https://reviews.apache.org/r/8681/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/#review16906
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On Feb. 19, 2013, 8:04 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8681/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 8:04 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this for my next review.
>
>
> Diffs
> -----
>
> third_party/libprocess/include/stout/os.hpp 712fb209061384b8c045875ef8898a6efd778514
>
> Diff: https://reviews.apache.org/r/8681/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/
-----------------------------------------------------------
(Updated Feb. 19, 2013, 8:04 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Rebased off trunk.
Description
-------
Needed this for my next review.
Diffs (updated)
-----
third_party/libprocess/include/stout/os.hpp 712fb209061384b8c045875ef8898a6efd778514
Diff: https://reviews.apache.org/r/8681/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 14, 2013, 1:43 a.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/os.hpp, line 401
> > <https://reviews.apache.org/r/8681/diff/3/?file=252338#file252338line401>
> >
> > What's the justification for changing this one? I don't really have objections, but I'd like to understand the thought behind this
I think its intuitive when you ask "if a link exists", to answer with whether that link exists, not whether the target of the link exists.
> On Feb. 14, 2013, 1:43 a.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/os.hpp, line 413
> > <https://reviews.apache.org/r/8681/diff/3/?file=252338#file252338line413>
> >
> > Here I'm not sure what to do either.. I feel like the caller would need to know the semantics they want in terms of links.
See my answer to Ben's question above.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/#review16551
-----------------------------------------------------------
On Jan. 28, 2013, 4:29 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8681/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2013, 4:29 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this for my next review.
>
>
> Diffs
> -----
>
> third_party/libprocess/include/stout/os.hpp fdba9cadc38251b192e9d395a0a1ab1f08327596
>
> Diff: https://reviews.apache.org/r/8681/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 14, 2013, 1:43 a.m., Ben Mahler wrote:
> >
Can you dig up other libraries as examples?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/#review16551
-----------------------------------------------------------
On Jan. 28, 2013, 4:29 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8681/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2013, 4:29 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this for my next review.
>
>
> Diffs
> -----
>
> third_party/libprocess/include/stout/os.hpp fdba9cadc38251b192e9d395a0a1ab1f08327596
>
> Diff: https://reviews.apache.org/r/8681/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 14, 2013, 1:43 a.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/os.hpp, line 413
> > <https://reviews.apache.org/r/8681/diff/3/?file=252338#file252338line413>
> >
> > Here I'm not sure what to do either.. I feel like the caller would need to know the semantics they want in terms of links.
>
> Vinod Kone wrote:
> See my answer to Ben's question above.
Ditto here. I think lstat is the right default, but I can see mistakes being made.
> On Feb. 14, 2013, 1:43 a.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/os.hpp, line 401
> > <https://reviews.apache.org/r/8681/diff/3/?file=252338#file252338line401>
> >
> > What's the justification for changing this one? I don't really have objections, but I'd like to understand the thought behind this
>
> Vinod Kone wrote:
> I think its intuitive when you ask "if a link exists", to answer with whether that link exists, not whether the target of the link exists.
Fair enough, I think lstat is better than stat, but I could still see mistakes being made. So I don't like forcing it either way.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/#review16551
-----------------------------------------------------------
On Jan. 28, 2013, 4:29 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8681/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2013, 4:29 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this for my next review.
>
>
> Diffs
> -----
>
> third_party/libprocess/include/stout/os.hpp fdba9cadc38251b192e9d395a0a1ab1f08327596
>
> Diff: https://reviews.apache.org/r/8681/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/#review16551
-----------------------------------------------------------
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/8681/#comment35122>
What's the justification for changing this one? I don't really have objections, but I'd like to understand the thought behind this
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/8681/#comment35123>
Here I'm not sure what to do either.. I feel like the caller would need to know the semantics they want in terms of links.
- Ben Mahler
On Jan. 28, 2013, 4:29 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8681/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2013, 4:29 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Needed this for my next review.
>
>
> Diffs
> -----
>
> third_party/libprocess/include/stout/os.hpp fdba9cadc38251b192e9d395a0a1ab1f08327596
>
> Diff: https://reviews.apache.org/r/8681/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/
-----------------------------------------------------------
(Updated Jan. 28, 2013, 4:29 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
fixed a bug in os::find()..with the recent s/lstat/stat
Description
-------
Needed this for my next review.
Diffs (updated)
-----
third_party/libprocess/include/stout/os.hpp fdba9cadc38251b192e9d395a0a1ab1f08327596
Diff: https://reviews.apache.org/r/8681/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part 6): Added os::islink and substituted
stat() with lstat() in os.hpp
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8681/
-----------------------------------------------------------
(Updated Jan. 24, 2013, 12:48 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benh's comments
Description
-------
Needed this for my next review.
Diffs (updated)
-----
third_party/libprocess/include/stout/os.hpp fdba9cadc38251b192e9d395a0a1ab1f08327596
Diff: https://reviews.apache.org/r/8681/diff/
Testing
-------
make check
Thanks,
Vinod Kone