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 2013/02/19 20:59:44 UTC
Re: Review Request: Slave Restart (Part Deux): Status Update Manager
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7212/
-----------------------------------------------------------
(Updated Feb. 19, 2013, 7:59 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
rebased off trunk.
Description
-------
added shutdownFramework
Minor fixes for open
Ben's comments
Refactoring SUM
Bens' comments
Status Update Manager
Rebased off latest trunk
Conflicts:
src/Makefile.am
src/common/protobuf_utils.hpp
src/common/utils.hpp
src/slave/slave.cpp
src/tests/protobuf_io_tests.cpp
src/tests/utils_tests.cpp
Diffs (updated)
-----
src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
src/common/protobuf_utils.hpp 69baab78c8db2d2d33ffbbe7f5e5fc80d65b0e1a
src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
src/local/local.cpp 3402029e07341212717e346a97e7cd30fa52ed51
src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
src/slave/status_update_manager.hpp PRE-CREATION
src/slave/status_update_manager.cpp PRE-CREATION
third_party/libprocess/include/process/timeout.hpp cac996070359a3e7ecdd8077af83c8c4cf9735fd
Diff: https://reviews.apache.org/r/7212/diff/
Testing
-------
make check.
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Deux): Status Update Manager
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 27, 2013, 10:31 p.m., Ben Mahler wrote:
> > src/common/type_utils.hpp, line 306
> > <https://reviews.apache.org/r/7212/diff/12/?file=261358#file261358line306>
> >
> > can you quote these ids, given we currently don't enforce no whitespace?
Throughout the code base we only quote executor ids because we know they can have space in them (our command executor does). I would be keep task ids unquoted for now, to be consistent. May be we can do a sweep later on of the code base if we decide to fix it.
> On Feb. 27, 2013, 10:31 p.m., Ben Mahler wrote:
> > src/messages/messages.proto, line 59
> > <https://reviews.apache.org/r/7212/diff/12/?file=261360#file261360line59>
> >
> > Hm.. how is sequence used? Is it perpetually incremented across recoveries? If so let's make this a uint64.
i think this is not used anymore! thanks for catching this.
> On Feb. 27, 2013, 10:31 p.m., Ben Mahler wrote:
> > src/slave/status_update_manager.hpp, line 69
> > <https://reviews.apache.org/r/7212/diff/12/?file=261361#file261361line69>
> >
> > s/Whether the update is handled successfully (e.g. checkpointed)./A Future of the update operation./
> >
> > You can then describe more return semantics if you like separately but I think if we want to use annotations like @return we should actually describe what we're returning in the first sentence (which is what Jie originally did before I refactored his return types).
> >
> > Whether sounds like a boolean to me, I think this pattern is present in the cgroups isolation module because the functions there used to return booleans before I refactored them to return Try's.
Changed it to "A future indicating whether the update is handled successfully (e.g. checkpointed)". I think that is more clear.
> On Feb. 27, 2013, 10:31 p.m., Ben Mahler wrote:
> > src/slave/status_update_manager.hpp, line 83
> > <https://reviews.apache.org/r/7212/diff/12/?file=261361#file261361line83>
> >
> > Can you add a ticket or review to track this? If there is one.
there isn't one :/
> On Feb. 27, 2013, 10:31 p.m., Ben Mahler wrote:
> > src/slave/status_update_manager.hpp, line 153
> > <https://reviews.apache.org/r/7212/diff/12/?file=261361#file261361line153>
> >
> > Can you maybe add a comment or note on what the uuid is for and why there can be a mismatch?
Actually I don't remember what the reasoning was. Made this a CHECK to be safe.
> On Feb. 27, 2013, 10:31 p.m., Ben Mahler wrote:
> > src/slave/status_update_manager.hpp, line 221
> > <https://reviews.apache.org/r/7212/diff/12/?file=261361#file261361line221>
> >
> > Why are you not using the protobuf write error? We've lost that information here!
good point. fixed.
> On Feb. 27, 2013, 10:31 p.m., Ben Mahler wrote:
> > src/slave/status_update_manager.cpp, line 169
> > <https://reviews.apache.org/r/7212/diff/12/?file=261362#file261362line169>
> >
> > Why are you checking the timeout is expired? Can you add a comment?
Because this is the first in the stream, as mentioned in the comment above. Timeout only gets set when we send an update. Let me know if thats not clear?
> On Feb. 27, 2013, 10:31 p.m., Ben Mahler wrote:
> > src/slave/status_update_manager.cpp, line 218
> > <https://reviews.apache.org/r/7212/diff/12/?file=261362#file261362line218>
> >
> > s/might/will
see above.
> On Feb. 27, 2013, 10:31 p.m., Ben Mahler wrote:
> > src/slave/status_update_manager.cpp, line 295
> > <https://reviews.apache.org/r/7212/diff/12/?file=261362#file261362line295>
> >
> > Can you make this an Option, that way you can CHECK_NOTNULL when the option is some in the caller?
I think thats a bit of too much overhead. NULL is a perfectly valid sentinel value to check against. Making it an option, doesn't add additional value but adds overhead.
> On Feb. 27, 2013, 10:31 p.m., Ben Mahler wrote:
> > src/slave/status_update_manager.cpp, line 208
> > <https://reviews.apache.org/r/7212/diff/12/?file=261362#file261362line208>
> >
> > s/might/will/
> >
> > Why do we allow this to happen? If it's hard to prevent, please elaborate in this comment.
We use 'might' for 'will', throughout our code base :)
While it might (:)) be easy to stop status updates reaching the SUM, there might be cases where the SUM can receive an ACK for a terminated stream. For eg: updates generated by the master. Added a comment for this.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7212/#review17166
-----------------------------------------------------------
On Feb. 23, 2013, 8:11 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7212/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2013, 8:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> added shutdownFramework
>
>
> Minor fixes for open
>
>
> Ben's comments
>
>
> Refactoring SUM
>
>
> Bens' comments
>
>
> Status Update Manager
>
> Rebased off latest trunk
>
> Conflicts:
> src/Makefile.am
> src/common/protobuf_utils.hpp
> src/common/utils.hpp
> src/slave/slave.cpp
> src/tests/protobuf_io_tests.cpp
> src/tests/utils_tests.cpp
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/common/protobuf_utils.hpp 69baab78c8db2d2d33ffbbe7f5e5fc80d65b0e1a
> src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
> src/local/local.cpp 3402029e07341212717e346a97e7cd30fa52ed51
> src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
> src/slave/status_update_manager.hpp PRE-CREATION
> src/slave/status_update_manager.cpp PRE-CREATION
> third_party/libprocess/include/process/timeout.hpp cac996070359a3e7ecdd8077af83c8c4cf9735fd
>
> Diff: https://reviews.apache.org/r/7212/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Deux): Status Update Manager
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7212/#review17166
-----------------------------------------------------------
src/common/type_utils.hpp
<https://reviews.apache.org/r/7212/#comment36383>
can you quote these ids, given we currently don't enforce no whitespace?
src/messages/messages.proto
<https://reviews.apache.org/r/7212/#comment36365>
Hm.. how is sequence used? Is it perpetually incremented across recoveries? If so let's make this a uint64.
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment36375>
s/Whether the update is handled successfully (e.g. checkpointed)./A Future of the update operation./
You can then describe more return semantics if you like separately but I think if we want to use annotations like @return we should actually describe what we're returning in the first sentence (which is what Jie originally did before I refactored his return types).
Whether sounds like a boolean to me, I think this pattern is present in the cgroups isolation module because the functions there used to return booleans before I refactored them to return Try's.
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment36377>
ditto
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment36379>
Can you add a ticket or review to track this? If there is one.
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment36381>
I don't see the need to make open a separate method, can you please inline it here?
1. open is confusing because it looks like a syscall at first glance.
2. it's not used anywhere else
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment36382>
ditto here for inlining, I think that will make this more clear and simplify the StatusUpdateStream struct as well!
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment36384>
Can you maybe add a comment or note on what the uuid is for and why there can be a mismatch?
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment36385>
If the StatusUpdateManager stream is in a failed state then yes IMO. Right now an error is indistinguishable from an empty pending queue.
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment36386>
Why are you not using the protobuf write error? We've lost that information here!
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment36387>
moving open() into the constructor obviates the need for this CHECK_SOME as well
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment36388>
moving close() into the destructor obviates the need for this CHECK_SOME
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/7212/#comment36399>
Can you kill the empty definition below and s/;/ {}/ here?
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/7212/#comment36400>
Looks like you can move this CHECK to be the first thing in this function.
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/7212/#comment36402>
Why are you checking the timeout is expired? Can you add a comment?
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/7212/#comment36404>
s/might/will/
Why do we allow this to happen? If it's hard to prevent, please elaborate in this comment.
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/7212/#comment36405>
s/might/will
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/7212/#comment36406>
This looks more appropriate as a None() option than a default timeout.
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/7212/#comment36407>
kill the quotes on "terminal"
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/7212/#comment36408>
If you made timeout an option you could CHECK here that timeout is some when the stream is not empty, which seems like a good validation of your internal state, right?
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/7212/#comment36409>
Can you make this an Option, that way you can CHECK_NOTNULL when the option is some in the caller?
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/7212/#comment36411>
This fits on one line.
third_party/libprocess/include/process/timeout.hpp
<https://reviews.apache.org/r/7212/#comment36412>
Ugh, I hate these double time comparisons but I'll be fixing that sometime soon :)
- Ben Mahler
On Feb. 23, 2013, 8:11 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7212/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2013, 8:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> added shutdownFramework
>
>
> Minor fixes for open
>
>
> Ben's comments
>
>
> Refactoring SUM
>
>
> Bens' comments
>
>
> Status Update Manager
>
> Rebased off latest trunk
>
> Conflicts:
> src/Makefile.am
> src/common/protobuf_utils.hpp
> src/common/utils.hpp
> src/slave/slave.cpp
> src/tests/protobuf_io_tests.cpp
> src/tests/utils_tests.cpp
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/common/protobuf_utils.hpp 69baab78c8db2d2d33ffbbe7f5e5fc80d65b0e1a
> src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
> src/local/local.cpp 3402029e07341212717e346a97e7cd30fa52ed51
> src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
> src/slave/status_update_manager.hpp PRE-CREATION
> src/slave/status_update_manager.cpp PRE-CREATION
> third_party/libprocess/include/process/timeout.hpp cac996070359a3e7ecdd8077af83c8c4cf9735fd
>
> Diff: https://reviews.apache.org/r/7212/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 2): Status Update Manager.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7212/
-----------------------------------------------------------
(Updated March 13, 2013, 6:15 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
s/part 1/part 2/
Summary (updated)
-----------------
Slave Restart (Part 2): Status Update Manager.
Description
-------
added shutdownFramework
Minor fixes for open
Ben's comments
Refactoring SUM
Bens' comments
Status Update Manager
Rebased off latest trunk
Conflicts:
src/Makefile.am
src/common/protobuf_utils.hpp
src/common/utils.hpp
src/slave/slave.cpp
src/tests/protobuf_io_tests.cpp
src/tests/utils_tests.cpp
Diffs
-----
src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
src/common/protobuf_utils.hpp 69baab78c8db2d2d33ffbbe7f5e5fc80d65b0e1a
src/common/type_utils.hpp 66c2dc17655123641433c078931acc9e769d5077
src/local/local.cpp 3402029e07341212717e346a97e7cd30fa52ed51
src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
src/slave/status_update_manager.hpp PRE-CREATION
src/slave/status_update_manager.cpp PRE-CREATION
third_party/libprocess/include/process/timeout.hpp cac996070359a3e7ecdd8077af83c8c4cf9735fd
Diff: https://reviews.apache.org/r/7212/diff/
Testing
-------
make check.
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part 1): Status Update Manager.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7212/
-----------------------------------------------------------
(Updated March 13, 2013, 6:10 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
rebased off trunk for posterity. no need for review.
Summary (updated)
-----------------
Slave Restart (Part 1): Status Update Manager.
Description
-------
added shutdownFramework
Minor fixes for open
Ben's comments
Refactoring SUM
Bens' comments
Status Update Manager
Rebased off latest trunk
Conflicts:
src/Makefile.am
src/common/protobuf_utils.hpp
src/common/utils.hpp
src/slave/slave.cpp
src/tests/protobuf_io_tests.cpp
src/tests/utils_tests.cpp
Diffs (updated)
-----
src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57
src/common/protobuf_utils.hpp 69baab78c8db2d2d33ffbbe7f5e5fc80d65b0e1a
src/common/type_utils.hpp 66c2dc17655123641433c078931acc9e769d5077
src/local/local.cpp 3402029e07341212717e346a97e7cd30fa52ed51
src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
src/slave/status_update_manager.hpp PRE-CREATION
src/slave/status_update_manager.cpp PRE-CREATION
third_party/libprocess/include/process/timeout.hpp cac996070359a3e7ecdd8077af83c8c4cf9735fd
Diff: https://reviews.apache.org/r/7212/diff/
Testing
-------
make check.
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Deux): Status Update Manager
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7212/
-----------------------------------------------------------
(Updated March 1, 2013, 10:39 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Changed Try<Option> to Result<>..duh.
Description
-------
added shutdownFramework
Minor fixes for open
Ben's comments
Refactoring SUM
Bens' comments
Status Update Manager
Rebased off latest trunk
Conflicts:
src/Makefile.am
src/common/protobuf_utils.hpp
src/common/utils.hpp
src/slave/slave.cpp
src/tests/protobuf_io_tests.cpp
src/tests/utils_tests.cpp
Diffs (updated)
-----
src/Makefile.am 851237cbf8db071d27de40c01d702514713b861a
src/common/protobuf_utils.hpp 69baab78c8db2d2d33ffbbe7f5e5fc80d65b0e1a
src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
src/local/local.cpp 3402029e07341212717e346a97e7cd30fa52ed51
src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
src/slave/status_update_manager.hpp PRE-CREATION
src/slave/status_update_manager.cpp PRE-CREATION
third_party/libprocess/include/process/timeout.hpp cac996070359a3e7ecdd8077af83c8c4cf9735fd
Diff: https://reviews.apache.org/r/7212/diff/
Testing
-------
make check.
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Deux): Status Update Manager
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7212/
-----------------------------------------------------------
(Updated March 1, 2013, 7:55 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's.
Description
-------
added shutdownFramework
Minor fixes for open
Ben's comments
Refactoring SUM
Bens' comments
Status Update Manager
Rebased off latest trunk
Conflicts:
src/Makefile.am
src/common/protobuf_utils.hpp
src/common/utils.hpp
src/slave/slave.cpp
src/tests/protobuf_io_tests.cpp
src/tests/utils_tests.cpp
Diffs (updated)
-----
src/Makefile.am 851237cbf8db071d27de40c01d702514713b861a
src/common/protobuf_utils.hpp 69baab78c8db2d2d33ffbbe7f5e5fc80d65b0e1a
src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
src/local/local.cpp 3402029e07341212717e346a97e7cd30fa52ed51
src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
src/slave/status_update_manager.hpp PRE-CREATION
src/slave/status_update_manager.cpp PRE-CREATION
third_party/libprocess/include/process/timeout.hpp cac996070359a3e7ecdd8077af83c8c4cf9735fd
Diff: https://reviews.apache.org/r/7212/diff/
Testing
-------
make check.
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Deux): Status Update Manager
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7212/
-----------------------------------------------------------
(Updated Feb. 23, 2013, 8:11 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benh's
Description
-------
added shutdownFramework
Minor fixes for open
Ben's comments
Refactoring SUM
Bens' comments
Status Update Manager
Rebased off latest trunk
Conflicts:
src/Makefile.am
src/common/protobuf_utils.hpp
src/common/utils.hpp
src/slave/slave.cpp
src/tests/protobuf_io_tests.cpp
src/tests/utils_tests.cpp
Diffs (updated)
-----
src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
src/common/protobuf_utils.hpp 69baab78c8db2d2d33ffbbe7f5e5fc80d65b0e1a
src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
src/local/local.cpp 3402029e07341212717e346a97e7cd30fa52ed51
src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
src/slave/status_update_manager.hpp PRE-CREATION
src/slave/status_update_manager.cpp PRE-CREATION
third_party/libprocess/include/process/timeout.hpp cac996070359a3e7ecdd8077af83c8c4cf9735fd
Diff: https://reviews.apache.org/r/7212/diff/
Testing
-------
make check.
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Deux): Status Update Manager
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 21, 2013, 11:51 p.m., Benjamin Hindman wrote:
> > src/slave/status_update_manager.hpp, line 179
> > <https://reviews.apache.org/r/7212/diff/11/?file=259875#file259875line179>
> >
> > Return an error or make a big todo at the top of this class to do this.
bit the bullet.
> On Feb. 21, 2013, 11:51 p.m., Benjamin Hindman wrote:
> > src/slave/status_update_manager.hpp, line 130
> > <https://reviews.apache.org/r/7212/diff/11/?file=259875#file259875line130>
> >
> > Methods in StatusUpdateStream should really be returning a Try (or Result if necessary). You can use the technique of an 'Option<bool> error' to capture when an error might have occurred and then return the error when any calls are made after an error has occured (i.e., a call to handle).
done.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7212/#review16888
-----------------------------------------------------------
On Feb. 19, 2013, 7:59 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7212/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 7:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> added shutdownFramework
>
>
> Minor fixes for open
>
>
> Ben's comments
>
>
> Refactoring SUM
>
>
> Bens' comments
>
>
> Status Update Manager
>
> Rebased off latest trunk
>
> Conflicts:
> src/Makefile.am
> src/common/protobuf_utils.hpp
> src/common/utils.hpp
> src/slave/slave.cpp
> src/tests/protobuf_io_tests.cpp
> src/tests/utils_tests.cpp
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/common/protobuf_utils.hpp 69baab78c8db2d2d33ffbbe7f5e5fc80d65b0e1a
> src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
> src/local/local.cpp 3402029e07341212717e346a97e7cd30fa52ed51
> src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
> src/slave/status_update_manager.hpp PRE-CREATION
> src/slave/status_update_manager.cpp PRE-CREATION
> third_party/libprocess/include/process/timeout.hpp cac996070359a3e7ecdd8077af83c8c4cf9735fd
>
> Diff: https://reviews.apache.org/r/7212/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Deux): Status Update Manager
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7212/#review16888
-----------------------------------------------------------
Ship it!
src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/7212/#comment35834>
s/reason/message/
src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/7212/#comment35833>
None().
src/common/type_utils.hpp
<https://reviews.apache.org/r/7212/#comment35835>
Each argument on newline please.
src/common/type_utils.hpp
<https://reviews.apache.org/r/7212/#comment35836>
Ditto.
src/common/type_utils.hpp
<https://reviews.apache.org/r/7212/#comment35838>
I'd love to kill this if it's not being used.
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment35847>
If open does not get called from outside constructor, we should make this private.
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment35842>
Methods in StatusUpdateStream should really be returning a Try (or Result if necessary). You can use the technique of an 'Option<bool> error' to capture when an error might have occurred and then return the error when any calls are made after an error has occured (i.e., a call to handle).
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment35841>
Wrap path in single quotes.
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment35848>
Likewise here ... if this is private to the implementation let's make it so.
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment35849>
This guy only really makes sense if open/close are private and your semantics are close only gets called via the destructor.
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment35850>
None() (here and everywhere else ... maybe just do one more commit at the end that takes care of this).
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment35851>
Single quotes.
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment35852>
Return an error or make a big todo at the top of this class to do this.
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/7212/#comment35853>
s/re-send/resend/
- Benjamin Hindman
On Feb. 19, 2013, 7:59 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7212/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 7:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> added shutdownFramework
>
>
> Minor fixes for open
>
>
> Ben's comments
>
>
> Refactoring SUM
>
>
> Bens' comments
>
>
> Status Update Manager
>
> Rebased off latest trunk
>
> Conflicts:
> src/Makefile.am
> src/common/protobuf_utils.hpp
> src/common/utils.hpp
> src/slave/slave.cpp
> src/tests/protobuf_io_tests.cpp
> src/tests/utils_tests.cpp
>
>
> Diffs
> -----
>
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/common/protobuf_utils.hpp 69baab78c8db2d2d33ffbbe7f5e5fc80d65b0e1a
> src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
> src/local/local.cpp 3402029e07341212717e346a97e7cd30fa52ed51
> src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f
> src/slave/status_update_manager.hpp PRE-CREATION
> src/slave/status_update_manager.cpp PRE-CREATION
> third_party/libprocess/include/process/timeout.hpp cac996070359a3e7ecdd8077af83c8c4cf9735fd
>
> Diff: https://reviews.apache.org/r/7212/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Vinod Kone
>
>