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 2012/12/25 09:47:28 UTC
Review Request: Slave Restart (Part 10): Properly guide users during
incompatible slave upgrade
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8763/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Description
-------
This checks slave info compatibility. Doesn't check version compatibility yet (because its not clear how we get that info).
Diffs
-----
src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
Diff: https://reviews.apache.org/r/8763/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part 10): Properly guide users during
incompatible slave upgrade
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 23, 2013, 11:56 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 1465
> > <https://reviews.apache.org/r/8763/diff/3/?file=259978#file259978line1465>
> >
> > Maybe "archive" it by renaming it rather than deleting it?
done. added a stout utility called tar() for archiving.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8763/#review16991
-----------------------------------------------------------
On Feb. 19, 2013, 8:08 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8763/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 8:08 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> This checks slave info compatibility. Doesn't check version compatibility yet (because its not clear how we get that info).
>
>
> Diffs
> -----
>
> src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
>
> Diff: https://reviews.apache.org/r/8763/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 10): Properly guide users during
incompatible slave upgrade
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8763/#review16991
-----------------------------------------------------------
Ship it!
src/slave/slave.cpp
<https://reviews.apache.org/r/8763/#comment36028>
Maybe "archive" it by renaming it rather than deleting it?
src/slave/slave.cpp
<https://reviews.apache.org/r/8763/#comment36029>
s/slaveinfo/SlaveInfo/
src/slave/slave.cpp
<https://reviews.apache.org/r/8763/#comment36030>
Should be a CopyFrom. Also, it is a bit of a hack, and the comment above should say something along the lines of needing to do this hack for the sake of _comparing_ the info created from the options/flags and what's been recovered.
- Benjamin Hindman
On Feb. 19, 2013, 8:08 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8763/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2013, 8:08 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> This checks slave info compatibility. Doesn't check version compatibility yet (because its not clear how we get that info).
>
>
> Diffs
> -----
>
> src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
>
> Diff: https://reviews.apache.org/r/8763/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 10): Properly guide users during
incompatible slave upgrade
Posted by Ben Mahler <be...@gmail.com>.
> On March 1, 2013, 7:25 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 1567
> > <https://reviews.apache.org/r/8763/diff/4/?file=263196#file263196line1567>
> >
> > can you put the isError case first? seems to the pattern we follow in general
>
> Vinod Kone wrote:
> Whenever possible, I like to keep the stuff that is more important, upfront.
I see, but the convention in mesos has been to handle the error upfront, which makes this harder to read, since it's breaking that convention.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8763/#review17252
-----------------------------------------------------------
On March 4, 2013, 1:34 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8763/
> -----------------------------------------------------------
>
> (Updated March 4, 2013, 1:34 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> This checks slave info compatibility. Doesn't check version compatibility yet (because its not clear how we get that info).
>
>
> Diffs
> -----
>
> src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
> src/slave/slave.cpp c13d268c5e0e107902f64e30304a18128927a571
> src/tests/slave_recovery_tests.cpp PRE-CREATION
> third_party/libprocess/third_party/stout/include/stout/os.hpp da14a8fb95b69684c899e04ca79a1ad8eb11b2b8
>
> Diff: https://reviews.apache.org/r/8763/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 10): Properly guide users during
incompatible slave upgrade
Posted by Vinod Kone <vi...@gmail.com>.
> On March 1, 2013, 7:25 p.m., Ben Mahler wrote:
> > src/common/type_utils.hpp, line 258
> > <https://reviews.apache.org/r/8763/diff/4/?file=263193#file263193line258>
> >
> > Again, I really hate these equality operators as they are really brittle to changes!!
> >
> > I don't see why you need this level of equality either.. you're saying changing the resources or attributes is incompatible?
> >
> > Changing the webui port is incompatible?
Yes. The current semantics are that a master associates a slave based on its id and info. If anything in the info changes, it is potentially a different slave.
> On March 1, 2013, 7:25 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 1567
> > <https://reviews.apache.org/r/8763/diff/4/?file=263196#file263196line1567>
> >
> > can you put the isError case first? seems to the pattern we follow in general
Whenever possible, I like to keep the stuff that is more important, upfront.
> On March 1, 2013, 7:25 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 1717
> > <https://reviews.apache.org/r/8763/diff/4/?file=263196#file263196line1717>
> >
> > is there a better way to capture incompatible upgrades, other than doing our own full equality comparison of the slave info?
As the TODO suggests, a version compatibility check is also useful.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8763/#review17252
-----------------------------------------------------------
On Feb. 27, 2013, 11:38 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8763/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2013, 11:38 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> This checks slave info compatibility. Doesn't check version compatibility yet (because its not clear how we get that info).
>
>
> Diffs
> -----
>
> src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
> src/slave/slave.cpp 8c2e1bfc363491c681177676f9dfe5f229276f7d
> src/tests/slave_recovery_tests.cpp PRE-CREATION
> third_party/libprocess/include/stout/os.hpp 32638ee273492550491b85223cda8e7a5bde7fa5
>
> Diff: https://reviews.apache.org/r/8763/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 10): Properly guide users during
incompatible slave upgrade
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8763/#review17252
-----------------------------------------------------------
Ship it!
src/common/type_utils.hpp
<https://reviews.apache.org/r/8763/#comment36681>
Again, I really hate these equality operators as they are really brittle to changes!!
I don't see why you need this level of equality either.. you're saying changing the resources or attributes is incompatible?
Changing the webui port is incompatible?
src/slave/slave.cpp
<https://reviews.apache.org/r/8763/#comment36682>
can you put the isError case first? seems to the pattern we follow in general
src/slave/slave.cpp
<https://reviews.apache.org/r/8763/#comment36683>
is there a better way to capture incompatible upgrades, other than doing our own full equality comparison of the slave info?
src/slave/slave.cpp
<https://reviews.apache.org/r/8763/#comment36679>
you need a newline before the proto as well
src/slave/slave.cpp
<https://reviews.apache.org/r/8763/#comment36680>
ditto
- Ben Mahler
On Feb. 27, 2013, 11:38 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8763/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2013, 11:38 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> This checks slave info compatibility. Doesn't check version compatibility yet (because its not clear how we get that info).
>
>
> Diffs
> -----
>
> src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
> src/slave/slave.cpp 8c2e1bfc363491c681177676f9dfe5f229276f7d
> src/tests/slave_recovery_tests.cpp PRE-CREATION
> third_party/libprocess/include/stout/os.hpp 32638ee273492550491b85223cda8e7a5bde7fa5
>
> Diff: https://reviews.apache.org/r/8763/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part 10): Properly guide users during
incompatible slave upgrade
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8763/
-----------------------------------------------------------
(Updated March 13, 2013, 6:14 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
rebased off trunk for posterity. no need for review.
Description
-------
This checks slave info compatibility. Doesn't check version compatibility yet (because its not clear how we get that info).
Diffs (updated)
-----
src/common/type_utils.hpp 66c2dc17655123641433c078931acc9e769d5077
src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
src/slave/slave.hpp 11f17aef6d5deefe83b2d4706e4b8b24adaac5f4
src/slave/slave.cpp 889e9fe7671c9b158486fd7b85fef139c4ab8c9b
third_party/libprocess/third_party/stout/include/stout/os.hpp da14a8fb95b69684c899e04ca79a1ad8eb11b2b8
Diff: https://reviews.apache.org/r/8763/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part 10): Properly guide users during
incompatible slave upgrade
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8763/
-----------------------------------------------------------
(Updated March 4, 2013, 1:34 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's.
Description
-------
This checks slave info compatibility. Doesn't check version compatibility yet (because its not clear how we get that info).
Diffs (updated)
-----
src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
src/slave/slave.cpp c13d268c5e0e107902f64e30304a18128927a571
src/tests/slave_recovery_tests.cpp PRE-CREATION
third_party/libprocess/third_party/stout/include/stout/os.hpp da14a8fb95b69684c899e04ca79a1ad8eb11b2b8
Diff: https://reviews.apache.org/r/8763/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part 10): Properly guide users during
incompatible slave upgrade
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8763/
-----------------------------------------------------------
(Updated Feb. 27, 2013, 11:38 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benh's.
Description
-------
This checks slave info compatibility. Doesn't check version compatibility yet (because its not clear how we get that info).
Diffs (updated)
-----
src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
src/slave/slave.hpp 7648c33230c1900eda7529045c5df9ccab105d47
src/slave/slave.cpp 8c2e1bfc363491c681177676f9dfe5f229276f7d
src/tests/slave_recovery_tests.cpp PRE-CREATION
third_party/libprocess/include/stout/os.hpp 32638ee273492550491b85223cda8e7a5bde7fa5
Diff: https://reviews.apache.org/r/8763/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part 10): Properly guide users during
incompatible slave upgrade
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8763/
-----------------------------------------------------------
(Updated Feb. 19, 2013, 8:08 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Properly deal with slave id, when comparing slave info compatiblity.
Description
-------
This checks slave info compatibility. Doesn't check version compatibility yet (because its not clear how we get that info).
Diffs (updated)
-----
src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
Diff: https://reviews.apache.org/r/8763/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part 10): Properly guide users during
incompatible slave upgrade
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8763/
-----------------------------------------------------------
(Updated Jan. 29, 2013, 11:09 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
rebased and some changes due to upstream reviews.
Description
-------
This checks slave info compatibility. Doesn't check version compatibility yet (because its not clear how we get that info).
Diffs (updated)
-----
src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00
src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
Diff: https://reviews.apache.org/r/8763/diff/
Testing
-------
make check
Thanks,
Vinod Kone