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 2013/01/16 20:33:55 UTC
Re: Review Request: Slave Restart (Part Tres): StatusUpdatesManager
integration into the slave
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7655/#review15397
-----------------------------------------------------------
Ship it!
Modulo comments below.
src/slave/paths.hpp
<https://reviews.apache.org/r/7655/#comment33141>
Can you log a warning here? I think this might be useful if the symlink fails because of this.
Try<Nothing> rm = os::rm(latest);
if (rm.isError()) {
LOG(WARNING) << "Failed to rm latest symlink '" << latest << "': " << rm.error();
}
I think CHECK_SOME is also ok here:
CHECK_SOME(os::rm(latest)) << "Failed to rm latest symlink '" << latest << "'";
src/slave/slave.cpp
<https://reviews.apache.org/r/7655/#comment33148>
Can you add a string when it's discarded?
s/update manager"/update manager: "
s/": " + future.failure()/future.failure()
s/""/"the future was discarded"
src/slave/slave.cpp
<https://reviews.apache.org/r/7655/#comment33149>
Any reason not to use the: "Failed to checkpoint..." log format?
src/slave/slave.cpp
<https://reviews.apache.org/r/7655/#comment33150>
In general I think "Could not" reads well, compared to "Couldn't", or even better for these messages:
"Failed to find framework..."
or
"Unknown framework..."
src/slave/slave.cpp
<https://reviews.apache.org/r/7655/#comment33151>
ditto
src/slave/slave.cpp
<https://reviews.apache.org/r/7655/#comment33152>
ditto my comment above for outputting discarded
src/slave/slave.cpp
<https://reviews.apache.org/r/7655/#comment33153>
s/size() == 0/empty()/
src/slave/slave.cpp
<https://reviews.apache.org/r/7655/#comment33154>
So they'll be transitioned by other code, or? Can you add a comment / pointer to where?
src/slave/slave.cpp
<https://reviews.apache.org/r/7655/#comment33155>
ditto discarded
src/tests/status_update_manager_tests.cpp
<https://reviews.apache.org/r/7655/#comment33157>
No easy way to WAIT_UNTIL instead of sleep?
src/tests/status_update_manager_tests.cpp
<https://reviews.apache.org/r/7655/#comment33158>
Kill the extra space:
s/fd =/fd =/
src/tests/status_update_manager_tests.cpp
<https://reviews.apache.org/r/7655/#comment33163>
I think this is cleaner if you stuff the record, result inside the while loop:
int updates = 0;
int acks = 0;
string uuid;
while (true) {
StatusUpdateRecord record;
Result<bool> result = protobuf::read(fd.get(), &record);
if (!result.isSome()) {
break;
}
if (record.type() == StatusUpdateRecord::UPDATE) {
EXPECT_EQ(TASK_RUNNING, record.update().status().state());
uuid = record.update().uuid();
updates++;
} else {
EXPECT_EQ(uuid, record.uuid());
acks++;
}
}
- Ben Mahler
On Dec. 14, 2012, 8:31 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7655/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2012, 8:31 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Integrated SUM into slave.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/slave/gc.cpp fe22f9b557215514e3d432d36af9dc0c377c437b
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
> src/slave/status_update_manager.hpp PRE-CREATION
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/status_update_manager_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/7655/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Tres): StatusUpdatesManager
integration into the slave
Posted by Vinod Kone <vi...@gmail.com>.
> On Jan. 16, 2013, 7:33 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 801
> > <https://reviews.apache.org/r/7655/diff/8/?file=238494#file238494line801>
> >
> > Any reason not to use the: "Failed to checkpoint..." log format?
>
> Vinod Kone wrote:
> I want to make sure it says that the failure is in the status update manager.
>
> Saying, Failed to checkpoint.....blah..by status update manager doesn't read nice.
>
> We might have to say.. "Status update manager failed to..".
>
> Also, I don't like "Failed to" for warnings. Sounds more ominous than intended.
>
> Ben Mahler wrote:
> Ok please use "could not" in that case.
done
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7655/#review15397
-----------------------------------------------------------
On Jan. 16, 2013, 11:10 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7655/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2013, 11:10 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Integrated SUM into slave.
>
>
> This addresses bug MESOS-110.
> https://issues.apache.org/jira/browse/MESOS-110
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/slave/gc.cpp fe22f9b557215514e3d432d36af9dc0c377c437b
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
> src/slave/status_update_manager.hpp PRE-CREATION
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/status_update_manager_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/7655/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Tres): StatusUpdatesManager
integration into the slave
Posted by Vinod Kone <vi...@gmail.com>.
> On Jan. 16, 2013, 7:33 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 801
> > <https://reviews.apache.org/r/7655/diff/8/?file=238494#file238494line801>
> >
> > Any reason not to use the: "Failed to checkpoint..." log format?
I want to make sure it says that the failure is in the status update manager.
Saying, Failed to checkpoint.....blah..by status update manager doesn't read nice.
We might have to say.. "Status update manager failed to..".
Also, I don't like "Failed to" for warnings. Sounds more ominous than intended.
> On Jan. 16, 2013, 7:33 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 903
> > <https://reviews.apache.org/r/7655/diff/8/?file=238494#file238494line903>
> >
> > In general I think "Could not" reads well, compared to "Couldn't", or even better for these messages:
> >
> > "Failed to find framework..."
> > or
> > "Unknown framework..."
I rephrased it as "Could not find..."
> On Jan. 16, 2013, 7:33 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 1271
> > <https://reviews.apache.org/r/7655/diff/8/?file=238494#file238494line1271>
> >
> > So they'll be transitioned by other code, or? Can you add a comment / pointer to where?
Actually, this logic is fixed in one of the latter reviews. So, I'll ignore this for now.
> On Jan. 16, 2013, 7:33 p.m., Ben Mahler wrote:
> > src/tests/status_update_manager_tests.cpp, line 212
> > <https://reviews.apache.org/r/7655/diff/8/?file=238497#file238497line212>
> >
> > No easy way to WAIT_UNTIL instead of sleep?
unfortunately, no. there are no side-effects that we can easily wait/depend on.
> On Jan. 16, 2013, 7:33 p.m., Ben Mahler wrote:
> > src/tests/status_update_manager_tests.cpp, line 220
> > <https://reviews.apache.org/r/7655/diff/8/?file=238497#file238497line220>
> >
> > Kill the extra space:
> > s/fd =/fd =/
good eye
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7655/#review15397
-----------------------------------------------------------
On Dec. 14, 2012, 8:31 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7655/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2012, 8:31 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Integrated SUM into slave.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/slave/gc.cpp fe22f9b557215514e3d432d36af9dc0c377c437b
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
> src/slave/status_update_manager.hpp PRE-CREATION
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/status_update_manager_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/7655/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Tres): StatusUpdatesManager
integration into the slave
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 16, 2013, 7:33 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 801
> > <https://reviews.apache.org/r/7655/diff/8/?file=238494#file238494line801>
> >
> > Any reason not to use the: "Failed to checkpoint..." log format?
>
> Vinod Kone wrote:
> I want to make sure it says that the failure is in the status update manager.
>
> Saying, Failed to checkpoint.....blah..by status update manager doesn't read nice.
>
> We might have to say.. "Status update manager failed to..".
>
> Also, I don't like "Failed to" for warnings. Sounds more ominous than intended.
Ok please use "could not" in that case.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7655/#review15397
-----------------------------------------------------------
On Jan. 16, 2013, 11:10 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7655/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2013, 11:10 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Integrated SUM into slave.
>
>
> This addresses bug MESOS-110.
> https://issues.apache.org/jira/browse/MESOS-110
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/slave/gc.cpp fe22f9b557215514e3d432d36af9dc0c377c437b
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018
> src/slave/status_update_manager.hpp PRE-CREATION
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/status_update_manager_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/7655/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>