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 2014/05/20 20:47:06 UTC
Review Request 21721: Added Owned::release().
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21721/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ben Mahler, and Jie Yu.
Repository: mesos-git
Description
-------
Will be used in Authorizer.
Diffs
-----
3rdparty/libprocess/include/process/owned.hpp aacd7b87c29fd31767621c475df98c819283cff9
3rdparty/libprocess/src/tests/owned_tests.cpp 234469b5e549fc7345ee2912c32c63aa3bb95a13
Diff: https://reviews.apache.org/r/21721/diff/
Testing
-------
libprocess make check
Thanks,
Vinod Kone
Re: Review Request 21721: Added Owned::release().
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21721/#review43516
-----------------------------------------------------------
Ship it!
Ship It!
- Dominic Hamon
On May 20, 2014, 11:47 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21721/
> -----------------------------------------------------------
>
> (Updated May 20, 2014, 11:47 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Will be used in Authorizer.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/owned.hpp aacd7b87c29fd31767621c475df98c819283cff9
> 3rdparty/libprocess/src/tests/owned_tests.cpp 234469b5e549fc7345ee2912c32c63aa3bb95a13
>
> Diff: https://reviews.apache.org/r/21721/diff/
>
>
> Testing
> -------
>
> libprocess make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 21721: Added Owned::release().
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21721/#review43521
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/include/process/owned.hpp
<https://reviews.apache.org/r/21721/#comment77711>
Ditto.
3rdparty/libprocess/include/process/owned.hpp
<https://reviews.apache.org/r/21721/#comment77710>
Modified the comments here and above:
"The ownership of this pointer has been lost"
3rdparty/libprocess/src/tests/owned_tests.cpp
<https://reviews.apache.org/r/21721/#comment77712>
EXPECT_EQ(NULL, owned.get());
3rdparty/libprocess/src/tests/owned_tests.cpp
<https://reviews.apache.org/r/21721/#comment77713>
Ditto.
- Jie Yu
On May 20, 2014, 6:47 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21721/
> -----------------------------------------------------------
>
> (Updated May 20, 2014, 6:47 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Will be used in Authorizer.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/owned.hpp aacd7b87c29fd31767621c475df98c819283cff9
> 3rdparty/libprocess/src/tests/owned_tests.cpp 234469b5e549fc7345ee2912c32c63aa3bb95a13
>
> Diff: https://reviews.apache.org/r/21721/diff/
>
>
> Testing
> -------
>
> libprocess make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 21721: Added Owned::release().
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21721/
-----------------------------------------------------------
(Updated May 22, 2014, 12:35 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Jie Yu.
Changes
-------
removed CHECK during race failures. NNFR.
Repository: mesos-git
Description
-------
Will be used in Authorizer.
Diffs (updated)
-----
3rdparty/libprocess/include/process/owned.hpp aacd7b87c29fd31767621c475df98c819283cff9
3rdparty/libprocess/src/tests/owned_tests.cpp 234469b5e549fc7345ee2912c32c63aa3bb95a13
Diff: https://reviews.apache.org/r/21721/diff/
Testing
-------
libprocess make check
Thanks,
Vinod Kone
Re: Review Request 21721: Added Owned::release().
Posted by Jie Yu <yu...@gmail.com>.
> On May 21, 2014, 6:09 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/owned.hpp, line 148
> > <https://reviews.apache.org/r/21721/diff/2/?file=585621#file585621line148>
> >
> > Not your code, but the semantics of this CHECK are a bit weird and since you're copying the semantics below I think we should clean this up. In particular, if 'data.get()' is NULL in the if check above then we return Shared<T>(NULL), but if we lose a race when we do __sync_fetch_and_and then we have a CHECK failure! Said another way, if we lose the race on line 147 we get a CHECK failure, but if we lose the race just after line 150 we return Shared<T>(NULL). Seems like we should always return Shared<T>(NULL).
Agreed with Ben. Let's clean it up.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21721/#review43543
-----------------------------------------------------------
On May 20, 2014, 8:40 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21721/
> -----------------------------------------------------------
>
> (Updated May 20, 2014, 8:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Will be used in Authorizer.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/owned.hpp aacd7b87c29fd31767621c475df98c819283cff9
> 3rdparty/libprocess/src/tests/owned_tests.cpp 234469b5e549fc7345ee2912c32c63aa3bb95a13
>
> Diff: https://reviews.apache.org/r/21721/diff/
>
>
> Testing
> -------
>
> libprocess make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 21721: Added Owned::release().
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21721/#review43543
-----------------------------------------------------------
3rdparty/libprocess/include/process/owned.hpp
<https://reviews.apache.org/r/21721/#comment77869>
Not your code, but the semantics of this CHECK are a bit weird and since you're copying the semantics below I think we should clean this up. In particular, if 'data.get()' is NULL in the if check above then we return Shared<T>(NULL), but if we lose a race when we do __sync_fetch_and_and then we have a CHECK failure! Said another way, if we lose the race on line 147 we get a CHECK failure, but if we lose the race just after line 150 we return Shared<T>(NULL). Seems like we should always return Shared<T>(NULL).
- Benjamin Hindman
On May 20, 2014, 8:40 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21721/
> -----------------------------------------------------------
>
> (Updated May 20, 2014, 8:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Will be used in Authorizer.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/owned.hpp aacd7b87c29fd31767621c475df98c819283cff9
> 3rdparty/libprocess/src/tests/owned_tests.cpp 234469b5e549fc7345ee2912c32c63aa3bb95a13
>
> Diff: https://reviews.apache.org/r/21721/diff/
>
>
> Testing
> -------
>
> libprocess make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 21721: Added Owned::release().
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21721/
-----------------------------------------------------------
(Updated May 20, 2014, 8:40 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Jie Yu.
Changes
-------
jie's comments. NNFR.
Repository: mesos-git
Description
-------
Will be used in Authorizer.
Diffs (updated)
-----
3rdparty/libprocess/include/process/owned.hpp aacd7b87c29fd31767621c475df98c819283cff9
3rdparty/libprocess/src/tests/owned_tests.cpp 234469b5e549fc7345ee2912c32c63aa3bb95a13
Diff: https://reviews.apache.org/r/21721/diff/
Testing
-------
libprocess make check
Thanks,
Vinod Kone