You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2012/09/20 01:34:28 UTC
Review Request: Adding error diagnostics to Try, Result, Future asserts.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7185/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Description
-------
See above.
Diffs
-----
third_party/libprocess/include/process/future.hpp bb0e366
third_party/libprocess/include/stout/result.hpp f6b92a0
third_party/libprocess/include/stout/try.hpp e865924
Diff: https://reviews.apache.org/r/7185/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Vinod review changes.
Posted by Ben Mahler <be...@gmail.com>.
> On Oct. 8, 2012, 7:31 p.m., Vinod Kone wrote:
> > third_party/libprocess/include/stout/try.hpp, line 64
> > <https://reviews.apache.org/r/7185/diff/1-2/?file=158651#file158651line64>
> >
> > still doesnt read well. the previous was better?
reverted
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7185/#review12237
-----------------------------------------------------------
On Oct. 8, 2012, 7:26 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7185/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2012, 7:26 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Vinod review changes.
>
>
> error diagnostics on try, result, future
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/future.hpp bb0e366b82d227cafd171d98013bd84045fba257
> third_party/libprocess/include/stout/result.hpp f6b92a0631a39a9d1fc33466eb868e05028a3928
> third_party/libprocess/include/stout/try.hpp e865924296633591af3c37b1e4aa3ba52955f199
>
> Diff: https://reviews.apache.org/r/7185/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Vinod review changes.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7185/#review12237
-----------------------------------------------------------
Ship it!
third_party/libprocess/include/stout/try.hpp
<https://reviews.apache.org/r/7185/#comment25971>
still doesnt read well. the previous was better?
- Vinod Kone
On Oct. 8, 2012, 7:26 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7185/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2012, 7:26 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> Vinod review changes.
>
>
> error diagnostics on try, result, future
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/future.hpp bb0e366b82d227cafd171d98013bd84045fba257
> third_party/libprocess/include/stout/result.hpp f6b92a0631a39a9d1fc33466eb868e05028a3928
> third_party/libprocess/include/stout/try.hpp e865924296633591af3c37b1e4aa3ba52955f199
>
> Diff: https://reviews.apache.org/r/7185/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Adding error diagnostics to Try, Result, Future asserts.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7185/
-----------------------------------------------------------
(Updated Oct. 9, 2012, 8:07 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Updated error strings.
Tested on linux.
Description
-------
see above
Diffs (updated)
-----
third_party/libprocess/include/process/future.hpp bb0e366b82d227cafd171d98013bd84045fba257
third_party/libprocess/include/stout/result.hpp f6b92a0631a39a9d1fc33466eb868e05028a3928
third_party/libprocess/include/stout/try.hpp e865924296633591af3c37b1e4aa3ba52955f199
Diff: https://reviews.apache.org/r/7185/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Adding error diagnostics to Try, Result, Future asserts.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7185/#review12249
-----------------------------------------------------------
Ship it!
Thanks Ben!
third_party/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/7185/#comment25990>
Add a CHECK(!isPending()) here first, since this is our bug. Then check isFailed and isDiscarded as you've done.
third_party/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/7185/#comment25991>
Future::get() and s/Future/future/
third_party/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/7185/#comment25992>
Ditto.
third_party/libprocess/include/stout/result.hpp
<https://reviews.apache.org/r/7185/#comment25993>
What about:
"Result::get() but state == ERROR:"
Same above for Future (and below for Try).
- Benjamin Hindman
On Oct. 8, 2012, 7:47 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7185/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2012, 7:47 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> see above
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/future.hpp bb0e366b82d227cafd171d98013bd84045fba257
> third_party/libprocess/include/stout/result.hpp f6b92a0631a39a9d1fc33466eb868e05028a3928
> third_party/libprocess/include/stout/try.hpp e865924296633591af3c37b1e4aa3ba52955f199
>
> Diff: https://reviews.apache.org/r/7185/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Adding error diagnostics to Try, Result, Future asserts.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7185/
-----------------------------------------------------------
(Updated Oct. 8, 2012, 7:47 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Need to fix my post-review settings to not change the summary.
Summary (updated)
-----------------
Adding error diagnostics to Try, Result, Future asserts.
Description
-------
see above
Diffs
-----
third_party/libprocess/include/process/future.hpp bb0e366b82d227cafd171d98013bd84045fba257
third_party/libprocess/include/stout/result.hpp f6b92a0631a39a9d1fc33466eb868e05028a3928
third_party/libprocess/include/stout/try.hpp e865924296633591af3c37b1e4aa3ba52955f199
Diff: https://reviews.apache.org/r/7185/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: changed error message on try
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7185/
-----------------------------------------------------------
(Updated Oct. 8, 2012, 7:42 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Summary (updated)
-----------------
changed error message on try
Description (updated)
-------
see above
Diffs (updated)
-----
third_party/libprocess/include/process/future.hpp bb0e366b82d227cafd171d98013bd84045fba257
third_party/libprocess/include/stout/result.hpp f6b92a0631a39a9d1fc33466eb868e05028a3928
third_party/libprocess/include/stout/try.hpp e865924296633591af3c37b1e4aa3ba52955f199
Diff: https://reviews.apache.org/r/7185/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Vinod review changes.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7185/
-----------------------------------------------------------
(Updated Oct. 8, 2012, 7:26 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
updated diff
Summary (updated)
-----------------
Vinod review changes.
Description (updated)
-------
Vinod review changes.
error diagnostics on try, result, future
Diffs (updated)
-----
third_party/libprocess/include/process/future.hpp bb0e366b82d227cafd171d98013bd84045fba257
third_party/libprocess/include/stout/result.hpp f6b92a0631a39a9d1fc33466eb868e05028a3928
third_party/libprocess/include/stout/try.hpp e865924296633591af3c37b1e4aa3ba52955f199
Diff: https://reviews.apache.org/r/7185/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request: Adding error diagnostics to Try, Result, Future asserts.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7185/#review12235
-----------------------------------------------------------
diff incoming (with post-review!)
third_party/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/7185/#comment25969>
I decided to keep this after adding the other states so that:
1. The code is more readable IMO, the state we're worried about in this case is that the future is still not ready. The other states are checked to print more information rather than control flow.
2. If the future is in an unknown state, we'll still abort since it's !isReady.
3. Paranoia, if someone adds a state (like benh discussed with Timeout), I'd want this code to fail fast (anything other than isReady will abort).
third_party/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/7185/#comment25964>
done, added errors for {DISCARDED, PENDING}
third_party/libprocess/include/stout/result.hpp
<https://reviews.apache.org/r/7185/#comment25967>
Nothing follows Result in this case.
third_party/libprocess/include/stout/try.hpp
<https://reviews.apache.org/r/7185/#comment25970>
also changed s/message/error()
- Ben Mahler
On Oct. 8, 2012, 6:26 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7185/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2012, 6:26 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See above.
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/future.hpp bb0e366
> third_party/libprocess/include/stout/result.hpp f6b92a0
> third_party/libprocess/include/stout/try.hpp e865924
>
> Diff: https://reviews.apache.org/r/7185/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Adding error diagnostics to Try, Result, Future asserts.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7185/#review12233
-----------------------------------------------------------
third_party/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/7185/#comment25959>
I dont think you need another !isReady() here.
third_party/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/7185/#comment25960>
what about discarded?
third_party/libprocess/include/stout/result.hpp
<https://reviews.apache.org/r/7185/#comment25961>
s/Result/Result: /
third_party/libprocess/include/stout/try.hpp
<https://reviews.apache.org/r/7185/#comment25963>
s/Try in ERROR:/ERROR Try:/
to be consistent
- Vinod Kone
On Oct. 8, 2012, 6:26 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7185/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2012, 6:26 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> See above.
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/future.hpp bb0e366
> third_party/libprocess/include/stout/result.hpp f6b92a0
> third_party/libprocess/include/stout/try.hpp e865924
>
> Diff: https://reviews.apache.org/r/7185/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Adding error diagnostics to Try, Result, Future asserts.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7185/
-----------------------------------------------------------
(Updated Oct. 8, 2012, 6:26 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
+vinod
Description
-------
See above.
Diffs
-----
third_party/libprocess/include/process/future.hpp bb0e366
third_party/libprocess/include/stout/result.hpp f6b92a0
third_party/libprocess/include/stout/try.hpp e865924
Diff: https://reviews.apache.org/r/7185/diff/
Testing
-------
make check
Thanks,
Ben Mahler