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/10/07 00:28:03 UTC

Review Request 26389: Fixed ~Authenticator() to discard the promise.

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

Review request for mesos, Ben Mahler, Dominic Hamon, and Jie Yu.


Bugs: MESOS-1866
    https://issues.apache.org/jira/browse/MESOS-1866


Repository: mesos-git


Description
-------

This fixes the race condition #2 as discussed in MESOS-1866.


Diffs
-----

  src/sasl/authenticatee.hpp dd68704c4851d28114dd632cd8e16882b2e30ebe 
  src/sasl/authenticator.hpp 35ab79449093e10877248b91ba7070e04c9cdd6f 
  src/tests/sasl_tests.cpp 59e1c95370879d4b006bfd80f16ce2a1f54a61df 

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


Testing
-------

Ensured that the test failed without the fix.

Ran the test 10K times in repitition with the fix.

make check


Thanks,

Vinod Kone

Re: Review Request 26389: Fixed ~Authenticator() to discard the promise.

Posted by Vinod Kone <vi...@gmail.com>.

> On Oct. 6, 2014, 10:30 p.m., Dominic Hamon wrote:
> > can this be removed when https://reviews.apache.org/r/25945/ lands?

not really. r/25945 deals with a different race where the promise was never associated (AuthenticatorProcess::authenticate() was never called).

AuthenticatorProcess.promise will not affected by r/25945.


> On Oct. 6, 2014, 10:30 p.m., Dominic Hamon wrote:
> > src/sasl/authenticatee.hpp, line 102
> > <https://reviews.apache.org/r/26389/diff/1/?file=714317#file714317line102>
> >
> >     this should probably be a failure rather than a discard. A discard is meant to be user-initiated.

discarded() calls promise.fail(). it is called discarded as it is the onDiscard() callback of the promise.


- Vinod


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


On Oct. 6, 2014, 10:28 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26389/
> -----------------------------------------------------------
> 
> (Updated Oct. 6, 2014, 10:28 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Dominic Hamon, and Jie Yu.
> 
> 
> Bugs: MESOS-1866
>     https://issues.apache.org/jira/browse/MESOS-1866
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This fixes the race condition #2 as discussed in MESOS-1866.
> 
> 
> Diffs
> -----
> 
>   src/sasl/authenticatee.hpp dd68704c4851d28114dd632cd8e16882b2e30ebe 
>   src/sasl/authenticator.hpp 35ab79449093e10877248b91ba7070e04c9cdd6f 
>   src/tests/sasl_tests.cpp 59e1c95370879d4b006bfd80f16ce2a1f54a61df 
> 
> Diff: https://reviews.apache.org/r/26389/diff/
> 
> 
> Testing
> -------
> 
> Ensured that the test failed without the fix.
> 
> Ran the test 10K times in repitition with the fix.
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 26389: Fixed ~Authenticator() to discard the promise.

Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26389/#review55589
-----------------------------------------------------------


can this be removed when https://reviews.apache.org/r/25945/ lands?


src/sasl/authenticatee.hpp
<https://reviews.apache.org/r/26389/#comment95946>

    this should probably be a failure rather than a discard. A discard is meant to be user-initiated.


- Dominic Hamon


On Oct. 6, 2014, 3:28 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26389/
> -----------------------------------------------------------
> 
> (Updated Oct. 6, 2014, 3:28 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Dominic Hamon, and Jie Yu.
> 
> 
> Bugs: MESOS-1866
>     https://issues.apache.org/jira/browse/MESOS-1866
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This fixes the race condition #2 as discussed in MESOS-1866.
> 
> 
> Diffs
> -----
> 
>   src/sasl/authenticatee.hpp dd68704c4851d28114dd632cd8e16882b2e30ebe 
>   src/sasl/authenticator.hpp 35ab79449093e10877248b91ba7070e04c9cdd6f 
>   src/tests/sasl_tests.cpp 59e1c95370879d4b006bfd80f16ce2a1f54a61df 
> 
> Diff: https://reviews.apache.org/r/26389/diff/
> 
> 
> Testing
> -------
> 
> Ensured that the test failed without the fix.
> 
> Ran the test 10K times in repitition with the fix.
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 26389: Fixed ~Authenticator() to discard the promise.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26389/#review55616
-----------------------------------------------------------



src/sasl/authenticatee.hpp
<https://reviews.apache.org/r/26389/#comment95976>

    s/Discard/Fail/



src/sasl/authenticator.hpp
<https://reviews.apache.org/r/26389/#comment95977>

    s/Discard/Fail/


- Jie Yu


On Oct. 6, 2014, 10:28 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26389/
> -----------------------------------------------------------
> 
> (Updated Oct. 6, 2014, 10:28 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Dominic Hamon, and Jie Yu.
> 
> 
> Bugs: MESOS-1866
>     https://issues.apache.org/jira/browse/MESOS-1866
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This fixes the race condition #2 as discussed in MESOS-1866.
> 
> 
> Diffs
> -----
> 
>   src/sasl/authenticatee.hpp dd68704c4851d28114dd632cd8e16882b2e30ebe 
>   src/sasl/authenticator.hpp 35ab79449093e10877248b91ba7070e04c9cdd6f 
>   src/tests/sasl_tests.cpp 59e1c95370879d4b006bfd80f16ce2a1f54a61df 
> 
> Diff: https://reviews.apache.org/r/26389/diff/
> 
> 
> Testing
> -------
> 
> Ensured that the test failed without the fix.
> 
> Ran the test 10K times in repitition with the fix.
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 26389: Fixed ~Authenticator() to discard the promise.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26389/#review55620
-----------------------------------------------------------

Ship it!



src/sasl/authenticatee.hpp
<https://reviews.apache.org/r/26389/#comment95984>

    Does this belong in finalize(), when the process is terminating?
    
    IMO this comment isn't necessary, we've used this finalize() Promise discard pattern many times in the code:
    
    TasksKiller
    NetworkProcess
    LogStorageProcess
    LogWriterProcess
    LogReaderProcess
    ExplicitPromiseProcess
    ImplicitPromiseProcess
    FillProcess
    WriteProcess
    CatchupProcess
    BulkCatchupProcess
    ExistenceChecker
    ... etc
    
    Not sure if we need such a verbose comment in all of these finalize() methods, this bug seems like an oversight to me.



src/sasl/authenticator.hpp
<https://reviews.apache.org/r/26389/#comment95985>

    Ditto comments above.


- Ben Mahler


On Oct. 7, 2014, 12:19 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26389/
> -----------------------------------------------------------
> 
> (Updated Oct. 7, 2014, 12:19 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Dominic Hamon, and Jie Yu.
> 
> 
> Bugs: MESOS-1866
>     https://issues.apache.org/jira/browse/MESOS-1866
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This fixes the race condition #2 as discussed in MESOS-1866.
> 
> 
> Diffs
> -----
> 
>   src/sasl/authenticatee.hpp dd68704c4851d28114dd632cd8e16882b2e30ebe 
>   src/sasl/authenticator.hpp 35ab79449093e10877248b91ba7070e04c9cdd6f 
>   src/tests/sasl_tests.cpp 59e1c95370879d4b006bfd80f16ce2a1f54a61df 
> 
> Diff: https://reviews.apache.org/r/26389/diff/
> 
> 
> Testing
> -------
> 
> Ensured that the test failed without the fix.
> 
> Ran the test 10K times in repitition with the fix.
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 26389: Fixed ~Authenticator() to discard the promise.

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26389/#review55622
-----------------------------------------------------------


Patch looks great!

Reviews applied: [26389]

All tests passed.

- Mesos ReviewBot


On Oct. 7, 2014, 12:19 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26389/
> -----------------------------------------------------------
> 
> (Updated Oct. 7, 2014, 12:19 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Dominic Hamon, and Jie Yu.
> 
> 
> Bugs: MESOS-1866
>     https://issues.apache.org/jira/browse/MESOS-1866
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This fixes the race condition #2 as discussed in MESOS-1866.
> 
> 
> Diffs
> -----
> 
>   src/sasl/authenticatee.hpp dd68704c4851d28114dd632cd8e16882b2e30ebe 
>   src/sasl/authenticator.hpp 35ab79449093e10877248b91ba7070e04c9cdd6f 
>   src/tests/sasl_tests.cpp 59e1c95370879d4b006bfd80f16ce2a1f54a61df 
> 
> Diff: https://reviews.apache.org/r/26389/diff/
> 
> 
> Testing
> -------
> 
> Ensured that the test failed without the fix.
> 
> Ran the test 10K times in repitition with the fix.
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 26389: Fixed ~Authenticator() to discard the promise.

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

(Updated Oct. 7, 2014, 12:19 a.m.)


Review request for mesos, Ben Mahler, Dominic Hamon, and Jie Yu.


Changes
-------

jie's


Bugs: MESOS-1866
    https://issues.apache.org/jira/browse/MESOS-1866


Repository: mesos-git


Description
-------

This fixes the race condition #2 as discussed in MESOS-1866.


Diffs (updated)
-----

  src/sasl/authenticatee.hpp dd68704c4851d28114dd632cd8e16882b2e30ebe 
  src/sasl/authenticator.hpp 35ab79449093e10877248b91ba7070e04c9cdd6f 
  src/tests/sasl_tests.cpp 59e1c95370879d4b006bfd80f16ce2a1f54a61df 

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


Testing
-------

Ensured that the test failed without the fix.

Ran the test 10K times in repitition with the fix.

make check


Thanks,

Vinod Kone