You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2012/04/23 20:29:08 UTC

Review Request: Updated Group and Group::Membership API.

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

Review request for mesos, John Sirois and Vinod Kone.


Summary
-------

(1) Renamed Group::info to Group::data (and associated changes).
(2) Added Group::Membership::watch to explicitly watch for a single membership's cancellation (and associated GroupProcess changes).
(3) Fixed a but in GroupProcess::update that was deleting a Watch object when also re-enqueueing it.
(4) Updated the GroupProcess destructor to fail all outstanding futures associated with the Group.


Diffs
-----

  src/log/network.hpp 0f7fefd 
  src/tests/zookeeper_tests.cpp a852103 
  src/zookeeper/group.hpp 695a8ab 
  src/zookeeper/group.cpp c9122f6 

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


Testing
-------


Thanks,

Benjamin

Re: Review Request: Updated Group and Group::Membership API.

Posted by Benjamin Hindman <be...@berkeley.edu>.

> On 2012-04-24 19:27:14, John Sirois wrote:
> > src/zookeeper/group.hpp, line 61
> > <https://reviews.apache.org/r/4844/diff/2/?file=104001#file104001line61>
> >
> >     I'm not finding a way to read this sanely:
> >     ~'returns true if cancelled.../ <-> membership.watch()
> >     
> >     IIUC all of these are more clear names:
> >     cancelled()
> >     withdrawn()
> >     discarded()
> >     abandoned()
> >     
> >     Each says to me the membership was chosen to be withdrawn by the user (vs via session expiry).
> >     
> >

Done.


> On 2012-04-24 19:27:14, John Sirois wrote:
> > src/zookeeper/group.hpp, line 102
> > <https://reviews.apache.org/r/4844/diff/2/?file=104001#file104001line102>
> >
> >     s/dat/data/

Done.


- Benjamin


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


On 2012-04-23 18:49:26, Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4844/
> -----------------------------------------------------------
> 
> (Updated 2012-04-23 18:49:26)
> 
> 
> Review request for mesos, John Sirois and Vinod Kone.
> 
> 
> Summary
> -------
> 
> (1) Renamed Group::info to Group::data (and associated changes).
> (2) Added Group::Membership::watch to explicitly watch for a single membership's cancellation (and associated GroupProcess changes).
> (3) Fixed a but in GroupProcess::update that was deleting a Watch object when also re-enqueueing it.
> (4) Updated the GroupProcess destructor to fail all outstanding futures associated with the Group.
> 
> 
> Diffs
> -----
> 
>   src/log/network.hpp 0f7fefd 
>   src/tests/zookeeper_tests.cpp a852103 
>   src/zookeeper/group.hpp 695a8ab 
>   src/zookeeper/group.cpp c9122f6 
> 
> Diff: https://reviews.apache.org/r/4844/diff
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin
> 
>

Re: Review Request: Updated Group and Group::Membership API.

Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4844/#review7182
-----------------------------------------------------------



src/zookeeper/group.hpp
<https://reviews.apache.org/r/4844/#comment15847>

    I'm not finding a way to read this sanely:
    ~'returns true if cancelled.../ <-> membership.watch()
    
    IIUC all of these are more clear names:
    cancelled()
    withdrawn()
    discarded()
    abandoned()
    
    Each says to me the membership was chosen to be withdrawn by the user (vs via session expiry).
    
    



src/zookeeper/group.hpp
<https://reviews.apache.org/r/4844/#comment15846>

    s/dat/data/


- John


On 2012-04-23 18:49:26, Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4844/
> -----------------------------------------------------------
> 
> (Updated 2012-04-23 18:49:26)
> 
> 
> Review request for mesos, John Sirois and Vinod Kone.
> 
> 
> Summary
> -------
> 
> (1) Renamed Group::info to Group::data (and associated changes).
> (2) Added Group::Membership::watch to explicitly watch for a single membership's cancellation (and associated GroupProcess changes).
> (3) Fixed a but in GroupProcess::update that was deleting a Watch object when also re-enqueueing it.
> (4) Updated the GroupProcess destructor to fail all outstanding futures associated with the Group.
> 
> 
> Diffs
> -----
> 
>   src/log/network.hpp 0f7fefd 
>   src/tests/zookeeper_tests.cpp a852103 
>   src/zookeeper/group.hpp 695a8ab 
>   src/zookeeper/group.cpp c9122f6 
> 
> Diff: https://reviews.apache.org/r/4844/diff
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin
> 
>

Re: Review Request: Updated Group and Group::Membership API.

Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4844/
-----------------------------------------------------------

(Updated 2012-05-01 00:14:31.743947)


Review request for mesos, John Sirois and Vinod Kone.


Changes
-------

Review comment updates.


Summary
-------

(1) Renamed Group::info to Group::data (and associated changes).
(2) Added Group::Membership::watch to explicitly watch for a single membership's cancellation (and associated GroupProcess changes).
(3) Fixed a but in GroupProcess::update that was deleting a Watch object when also re-enqueueing it.
(4) Updated the GroupProcess destructor to fail all outstanding futures associated with the Group.


Diffs (updated)
-----

  src/log/network.hpp 6ce6a53 
  src/tests/zookeeper_tests.cpp a852103 
  src/zookeeper/group.hpp 695a8ab 
  src/zookeeper/group.cpp da2e147 

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


Testing
-------

make check


Thanks,

Benjamin

Re: Review Request: Updated Group and Group::Membership API.

Posted by Benjamin Hindman <be...@berkeley.edu>.

> On 2012-04-24 21:32:34, Vinod Kone wrote:
> > src/zookeeper/group.cpp, line 458
> > <https://reviews.apache.org/r/4844/diff/2/?file=104002#file104002line458>
> >
> >     consider foreachpair

Done.


> On 2012-04-24 21:32:34, Vinod Kone wrote:
> > src/zookeeper/group.cpp, line 468
> > <https://reviews.apache.org/r/4844/diff/2/?file=104002#file104002line468>
> >
> >     s/onowned/unowned/

Done.


> On 2012-04-24 21:32:34, Vinod Kone wrote:
> > src/zookeeper/group.cpp, line 585
> > <https://reviews.apache.org/r/4844/diff/2/?file=104002#file104002line585>
> >
> >     should this be == 1?

Yes, I like the explicitness.


> On 2012-04-24 21:32:34, Vinod Kone wrote:
> > src/zookeeper/group.cpp, line 672
> > <https://reviews.apache.org/r/4844/diff/2/?file=104002#file104002line672>
> >
> >     consider foreachpair here and below

Done.


- Benjamin


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


On 2012-04-23 18:49:26, Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4844/
> -----------------------------------------------------------
> 
> (Updated 2012-04-23 18:49:26)
> 
> 
> Review request for mesos, John Sirois and Vinod Kone.
> 
> 
> Summary
> -------
> 
> (1) Renamed Group::info to Group::data (and associated changes).
> (2) Added Group::Membership::watch to explicitly watch for a single membership's cancellation (and associated GroupProcess changes).
> (3) Fixed a but in GroupProcess::update that was deleting a Watch object when also re-enqueueing it.
> (4) Updated the GroupProcess destructor to fail all outstanding futures associated with the Group.
> 
> 
> Diffs
> -----
> 
>   src/log/network.hpp 0f7fefd 
>   src/tests/zookeeper_tests.cpp a852103 
>   src/zookeeper/group.hpp 695a8ab 
>   src/zookeeper/group.cpp c9122f6 
> 
> Diff: https://reviews.apache.org/r/4844/diff
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin
> 
>

Re: Review Request: Updated Group and Group::Membership API.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4844/#review7189
-----------------------------------------------------------

Ship it!



src/zookeeper/group.cpp
<https://reviews.apache.org/r/4844/#comment15872>

    consider foreachpair



src/zookeeper/group.cpp
<https://reviews.apache.org/r/4844/#comment15873>

    s/onowned/unowned/



src/zookeeper/group.cpp
<https://reviews.apache.org/r/4844/#comment15874>

    should this be == 1?



src/zookeeper/group.cpp
<https://reviews.apache.org/r/4844/#comment15876>

    consider foreachpair here and below


- Vinod


On 2012-04-23 18:49:26, Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4844/
> -----------------------------------------------------------
> 
> (Updated 2012-04-23 18:49:26)
> 
> 
> Review request for mesos, John Sirois and Vinod Kone.
> 
> 
> Summary
> -------
> 
> (1) Renamed Group::info to Group::data (and associated changes).
> (2) Added Group::Membership::watch to explicitly watch for a single membership's cancellation (and associated GroupProcess changes).
> (3) Fixed a but in GroupProcess::update that was deleting a Watch object when also re-enqueueing it.
> (4) Updated the GroupProcess destructor to fail all outstanding futures associated with the Group.
> 
> 
> Diffs
> -----
> 
>   src/log/network.hpp 0f7fefd 
>   src/tests/zookeeper_tests.cpp a852103 
>   src/zookeeper/group.hpp 695a8ab 
>   src/zookeeper/group.cpp c9122f6 
> 
> Diff: https://reviews.apache.org/r/4844/diff
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin
> 
>

Re: Review Request: Updated Group and Group::Membership API.

Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4844/
-----------------------------------------------------------

(Updated 2012-04-23 18:49:26.163459)


Review request for mesos, John Sirois and Vinod Kone.


Changes
-------

Added comments for Group::Membership::watch.


Summary
-------

(1) Renamed Group::info to Group::data (and associated changes).
(2) Added Group::Membership::watch to explicitly watch for a single membership's cancellation (and associated GroupProcess changes).
(3) Fixed a but in GroupProcess::update that was deleting a Watch object when also re-enqueueing it.
(4) Updated the GroupProcess destructor to fail all outstanding futures associated with the Group.


Diffs (updated)
-----

  src/log/network.hpp 0f7fefd 
  src/tests/zookeeper_tests.cpp a852103 
  src/zookeeper/group.hpp 695a8ab 
  src/zookeeper/group.cpp c9122f6 

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


Testing
-------

make check


Thanks,

Benjamin

Re: Review Request: Updated Group and Group::Membership API.

Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4844/
-----------------------------------------------------------

(Updated 2012-04-23 18:29:28.926567)


Review request for mesos, John Sirois and Vinod Kone.


Changes
-------

Updated "Testing Done:".


Summary
-------

(1) Renamed Group::info to Group::data (and associated changes).
(2) Added Group::Membership::watch to explicitly watch for a single membership's cancellation (and associated GroupProcess changes).
(3) Fixed a but in GroupProcess::update that was deleting a Watch object when also re-enqueueing it.
(4) Updated the GroupProcess destructor to fail all outstanding futures associated with the Group.


Diffs
-----

  src/log/network.hpp 0f7fefd 
  src/tests/zookeeper_tests.cpp a852103 
  src/zookeeper/group.hpp 695a8ab 
  src/zookeeper/group.cpp c9122f6 

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


Testing (updated)
-------

make check


Thanks,

Benjamin