You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2013/10/31 05:59:59 UTC
Review Request 15112: Supported upgrading from Shared to Owned.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15112/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ben Mahler, Vinod Kone, and Jiang Yan Xu.
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/include/process/shared.hpp faa11cc
3rdparty/libprocess/src/tests/shared_tests.cpp e520b4f
Diff: https://reviews.apache.org/r/15112/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 15112: Supported upgrading from Shared to Owned.
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 31, 2013, 6:19 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/shared.hpp, line 175
> > <https://reviews.apache.org/r/15112/diff/2/?file=374598#file374598line175>
> >
> > Do we need any memory fencing here?
I thought about this when coding. My conclusion is that we should not have a memory consistency issue here. Even if some reads after the read of 'upgraded' here are reordered (i.e., executed before the read of 'upgraded'), it won't cause any problem because we don't rely on 'upgraded' to establish order between reads and writes to any other locations.
Thoughts?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15112/#review27860
-----------------------------------------------------------
On Oct. 31, 2013, 5:05 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15112/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2013, 5:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Vinod Kone, and Jiang Yan Xu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/shared.hpp faa11cc
> 3rdparty/libprocess/src/tests/shared_tests.cpp e520b4f
>
> Diff: https://reviews.apache.org/r/15112/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 15112: Supported upgrading from Shared to Owned.
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 31, 2013, 6:19 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/shared.hpp, line 175
> > <https://reviews.apache.org/r/15112/diff/2/?file=374598#file374598line175>
> >
> > Do we need any memory fencing here?
>
> Jie Yu wrote:
> I thought about this when coding. My conclusion is that we should not have a memory consistency issue here. Even if some reads after the read of 'upgraded' here are reordered (i.e., executed before the read of 'upgraded'), it won't cause any problem because we don't rely on 'upgraded' to establish order between reads and writes to any other locations.
>
> Thoughts?
Marked 'upgraded' as volatile to prevent compiler from caching its value.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15112/#review27860
-----------------------------------------------------------
On Oct. 31, 2013, 5:13 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15112/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2013, 5:13 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Vinod Kone, and Jiang Yan Xu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/shared.hpp faa11cc
> 3rdparty/libprocess/src/tests/shared_tests.cpp e520b4f
>
> Diff: https://reviews.apache.org/r/15112/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 15112: Supported upgrading from Shared to Owned.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15112/#review27860
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/include/process/shared.hpp
<https://reviews.apache.org/r/15112/#comment54258>
Not sure I'd expose this one, do we need it yet?
3rdparty/libprocess/include/process/shared.hpp
<https://reviews.apache.org/r/15112/#comment54257>
Not sure I'd expose this one, do we need it yet?
3rdparty/libprocess/include/process/shared.hpp
<https://reviews.apache.org/r/15112/#comment54251>
: data(new Data(CHECK_NOTNULL(t))) {}
Also, are these the same semantics as shared_ptr? I.e., can you not instantiate a shared_ptr with NULL to start?
3rdparty/libprocess/include/process/shared.hpp
<https://reviews.apache.org/r/15112/#comment54252>
How about two checks?
CHECK(data);
CHECK_NOTNULL(data->t);
I think the implicit bool operator is a bit weird though and I'd almost prefer:
CHECK_NOTNULL(data.get());
CHECK_NOTNULL(data->t);
Thoughts?
3rdparty/libprocess/include/process/shared.hpp
<https://reviews.apache.org/r/15112/#comment54253>
And 't' can be NULL here right?
3rdparty/libprocess/include/process/shared.hpp
<https://reviews.apache.org/r/15112/#comment54254>
Same thoughts here as above re: implicit bool operator.
3rdparty/libprocess/include/process/shared.hpp
<https://reviews.apache.org/r/15112/#comment54255>
s/upgrade attempt is detected/upgrade is already being performed/
3rdparty/libprocess/include/process/shared.hpp
<https://reviews.apache.org/r/15112/#comment54256>
Nice!
3rdparty/libprocess/include/process/shared.hpp
<https://reviews.apache.org/r/15112/#comment54259>
Do we need any memory fencing here?
3rdparty/libprocess/src/tests/shared_tests.cpp
<https://reviews.apache.org/r/15112/#comment54266>
I think we should treat a Shared<T> just like we do a T* and since we don't do 'if (t) {' I don't think we should support the bool operator on Shared right now but instead require 'if (t.get() != NULL) {'. I think being explicit here is better.
- Benjamin Hindman
On Oct. 31, 2013, 5:05 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15112/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2013, 5:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Vinod Kone, and Jiang Yan Xu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/shared.hpp faa11cc
> 3rdparty/libprocess/src/tests/shared_tests.cpp e520b4f
>
> Diff: https://reviews.apache.org/r/15112/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 15112: Supported upgrading from Shared to Owned.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15112/#review27975
-----------------------------------------------------------
Ship it!
I'll get this committed!
- Benjamin Hindman
On Oct. 31, 2013, 8:25 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15112/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2013, 8:25 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Vinod Kone, and Jiang Yan Xu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/shared.hpp faa11cc
> 3rdparty/libprocess/src/tests/shared_tests.cpp e520b4f
>
> Diff: https://reviews.apache.org/r/15112/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 15112: Supported upgrading from Shared to Owned.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15112/
-----------------------------------------------------------
(Updated Oct. 31, 2013, 8:25 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Vinod Kone, and Jiang Yan Xu.
Changes
-------
BenH's comments.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/shared.hpp faa11cc
3rdparty/libprocess/src/tests/shared_tests.cpp e520b4f
Diff: https://reviews.apache.org/r/15112/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 15112: Supported upgrading from Shared to Owned.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15112/#review27942
-----------------------------------------------------------
3rdparty/libprocess/include/process/shared.hpp
<https://reviews.apache.org/r/15112/#comment54387>
An alternative strategy would be to just return an Owned<T>(NULL)? What's your reasoning on these semantics instead? A comment or different semantics please.
3rdparty/libprocess/src/tests/shared_tests.cpp
<https://reviews.apache.org/r/15112/#comment54385>
In the interest in keeping this consistent, can we s/sp/shared/ and s/sp2/shared2/ and s/op/owned/ please?
- Benjamin Hindman
On Oct. 31, 2013, 7:01 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15112/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2013, 7:01 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Vinod Kone, and Jiang Yan Xu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/shared.hpp faa11cc
> 3rdparty/libprocess/src/tests/shared_tests.cpp e520b4f
>
> Diff: https://reviews.apache.org/r/15112/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 15112: Supported upgrading from Shared to Owned.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15112/
-----------------------------------------------------------
(Updated Oct. 31, 2013, 7:01 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Vinod Kone, and Jiang Yan Xu.
Changes
-------
BenM's comment.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/shared.hpp faa11cc
3rdparty/libprocess/src/tests/shared_tests.cpp e520b4f
Diff: https://reviews.apache.org/r/15112/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 15112: Supported upgrading from Shared to Owned.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15112/#review27935
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/include/process/shared.hpp
<https://reviews.apache.org/r/15112/#comment54371>
s/empty/NULL/ ?
- Ben Mahler
On Oct. 31, 2013, 6:11 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15112/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2013, 6:11 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Vinod Kone, and Jiang Yan Xu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/shared.hpp faa11cc
> 3rdparty/libprocess/src/tests/shared_tests.cpp e520b4f
>
> Diff: https://reviews.apache.org/r/15112/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 15112: Supported upgrading from Shared to Owned.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15112/
-----------------------------------------------------------
(Updated Oct. 31, 2013, 6:11 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Vinod Kone, and Jiang Yan Xu.
Changes
-------
Using CHECK_NOTNULL.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/shared.hpp faa11cc
3rdparty/libprocess/src/tests/shared_tests.cpp e520b4f
Diff: https://reviews.apache.org/r/15112/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 15112: Supported upgrading from Shared to Owned.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15112/
-----------------------------------------------------------
(Updated Oct. 31, 2013, 5:13 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Vinod Kone, and Jiang Yan Xu.
Changes
-------
BenH's comments. Added more tests.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/shared.hpp faa11cc
3rdparty/libprocess/src/tests/shared_tests.cpp e520b4f
Diff: https://reviews.apache.org/r/15112/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 15112: Supported upgrading from Shared to Owned.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15112/
-----------------------------------------------------------
(Updated Oct. 31, 2013, 5:05 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Vinod Kone, and Jiang Yan Xu.
Changes
-------
Added more CHECKs.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/shared.hpp faa11cc
3rdparty/libprocess/src/tests/shared_tests.cpp e520b4f
Diff: https://reviews.apache.org/r/15112/diff/
Testing
-------
make check
Thanks,
Jie Yu