mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 17686: Updated Mesos to use new libprocess discard semantics.
Date Tue, 18 Feb 2014 08:45:13 GMT


> On Feb. 8, 2014, 12:10 a.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, lines 1093-1094
> > <https://reviews.apache.org/r/17686/diff/3/?file=470406#file470406line1093>
> >
> >       virtual void finalize()
> >       {
> >         // Unregister the eventfd if needed.
> >         if (eventfd.isSome()) {
> >           Try<Nothing> unregister = internal::unregisterNotifier(eventfd.get());
> >           if (unregister.isError()) {
> >             LOG(ERROR) << "Failed to unregistering eventfd: " << unregister.error();
> >           }
> >         }
> >       }

I'm assuming you were suggesting I drop the 'reading.discard()', which I don't want to do
(see comments above). Or did I miss something?


> On Feb. 8, 2014, 12:10 a.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, lines 1069-1070
> > <https://reviews.apache.org/r/17686/diff/3/?file=470406#file470406line1069>
> >
> >     Move this to the bottom and:
> >     
> >     // Discard reading if the caller discards.
> >     promise.future().onDiscard(lambda::bind(__discard, reading));

I was trying to not do a ton of code motion in this review. In this case, we were always terminating
a process on a discarded event and propagating discard on 'reading' in 'finalize' so I kept
these semantics but just added the 'promise.discard()' since the original semantics effectively
discarded both the promise and the future. I did add a TODO that we should actually wait until
the result of 'reading' but I'd prefer to do that, and any other code motion, in a subsequent
review.


> On Feb. 8, 2014, 12:10 a.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, lines 1119-1122
> > <https://reviews.apache.org/r/17686/diff/3/?file=470406#file470406line1119>
> >
> >     Now we discard the promise only if reading is discarded:
> >     
> >     
> >     if (reading.isDiscarded()) {
> >       promise.discard();
> >     } else if (reading.isFailed()) {
> >        ...

I updated here and other places to discard the promise if our underlying future was discarded.
I originally was trying to keep as much of the original semantics of the code as possible
and only make changes if there was an actual bug introduced by the new discard semantics,
but I like eliminating these isDiscarded CHECKs so I went ahead and took care of them! ;)


> On Feb. 8, 2014, 12:10 a.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, lines 1516-1517
> > <https://reviews.apache.org/r/17686/diff/3/?file=470406#file470406line1516>
> >
> >     // Discard the chain when the caller discards.
> >     promise.future().onDiscard(lambda::bind(__discard, chain));

Like above, I'm going to keep the same semantics as the original code when it comes to discarding
here and terminate the process doing a Promise::discard in 'finalize' (if necessary).


> On Feb. 8, 2014, 12:10 a.m., Ben Mahler wrote:
> > src/sasl/authenticatee.hpp, line 323
> > <https://reviews.apache.org/r/17686/diff/3/?file=470412#file470412line323>
> >
> >     This seems a little strange:
> >     
> >     STATUS=ERROR -> fail()
> >     STATUS=DISCARDED -> fail() (why not discard())?
> >     
> >     Also this looks a little odd to the callers as well, since they discard but
a failure will be the result.

Discarding the future will cause the future to fail if it hasn't already completed since we
have already started the authentication procedure and can't reliably cancel. (<-- This
is what I added to the API comment as well!)

More generally, we can't always assume that calling Future::discard will in fact lead to the
future being discarded, it will greatly depend on the circumstance. In this case, I don't
want to give the illusion that authentication was discarded (or didn't start), hence the failure.


- Benjamin


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


On Feb. 5, 2014, 11:41 p.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17686/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2014, 11:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Ben Mahler, Ian Downes, Jie Yu, Niklas Nielsen, TILL
TOENSHOFF, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/java/jni/org_apache_mesos_state_AbstractState.cpp 2ee0b1b631b80ec783e6bce683cdeaa77e56b2aa

>   src/linux/cgroups.cpp 19ab1f348191ab0315271477b206aa8c6456fd5a 
>   src/log/catchup.cpp 4ee32f285f77eb2de661e22a301b743bb8a06f9c 
>   src/log/consensus.cpp b89673a3b8f233e901eaf9ae69a9979099f4eb73 
>   src/log/recover.cpp bb32e51e1172a8b32ac74be9848c1e72db5cefa0 
>   src/master/detector.cpp 2b169c551affe7acd2feac7806a27b46eb99bb88 
>   src/master/registrar.cpp 915885a160f790399e8185c28c6e6555af1ee76e 
>   src/sasl/authenticatee.hpp f1a677f8aed0979f958e51f85e0a8210a03bd343 
>   src/sasl/authenticator.hpp 1478f6771b424555c34586a0d61f208dc15b0e7d 
>   src/slave/gc.cpp 405350bf8f498d2e59e9e6b4c4c19b7bdaa974de 
>   src/zookeeper/contender.cpp 6710da4e64fc0a43c1eabfc0f39fb0133c13df14 
>   src/zookeeper/group.cpp ecb6c002e8194b8d67e262826d988f747414f9f3 
> 
> Diff: https://reviews.apache.org/r/17686/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message