You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2017/05/24 22:13:51 UTC
Review Request 59545: Added a test to verify the agent flag
'executor_reregistration_timeout.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59545/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
Bugs: MESOS-7540
https://issues.apache.org/jira/browse/MESOS-7540
Repository: mesos
Description
-------
This patch adds a test to verify the correct behavior of
the agent flag 'executor_reregistration_timeout.
Diffs
-----
src/tests/slave_tests.cpp d124d594fb4f20db3f5dacbc7482d97161cca2df
Diff: https://reviews.apache.org/r/59545/diff/1/
Testing
-------
bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1 --gtest_break_on_failure
Thanks,
Greg Mann
Re: Review Request 59545: Added a test to verify the agent flag
'executor_reregistration_timeout'.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59545/#review176650
-----------------------------------------------------------
Sorry, I couldn't resist a few pedantic coding style suggestions... I also realize that I'm complaining about things that occur in other tests as well :)
src/tests/slave_tests.cpp
Lines 7255 (patched)
<https://reviews.apache.org/r/59545/#comment250049>
Do we actually expect subsequent offers in the test, or is this just coding defensively? (If the latter, I'm not sure it is good practice, since it might hide bugs or unexpected behavioral changes.)
src/tests/slave_tests.cpp
Lines 7259 (patched)
<https://reviews.apache.org/r/59545/#comment250046>
Do we actually need to wait for a batch allocation here?
IMO it would be clearer to first prompt the agent to register (before connecting the framework); then when the framework registers, it should immediately receive an offer.
src/tests/slave_tests.cpp
Lines 7263 (patched)
<https://reviews.apache.org/r/59545/#comment250051>
Style-wise, I'd prefer `ASSERT_FALSE(offers.empty())`.
src/tests/slave_tests.cpp
Lines 7265 (patched)
<https://reviews.apache.org/r/59545/#comment250050>
Style-wise, I'd prefer `offers->front()`.
src/tests/slave_tests.cpp
Lines 7322 (patched)
<https://reviews.apache.org/r/59545/#comment250053>
Won't be marked `TASK_UNREACHABLE` because the framework is not PA; arguably confusing to suggest that in the comment.
src/tests/slave_tests.cpp
Lines 7335 (patched)
<https://reviews.apache.org/r/59545/#comment250056>
As above, if we don't expect additional status updates, not sure it is good practice to add the `WillRepeatedly` expectation.
src/tests/slave_tests.cpp
Lines 7340 (patched)
<https://reviews.apache.org/r/59545/#comment250048>
I'd argue for `EXPECT` here, not `ASSERT`.
- Neil Conway
On May 30, 2017, 11:29 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59545/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 11:29 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-7540
> https://issues.apache.org/jira/browse/MESOS-7540
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a test to verify the correct behavior of
> the agent flag 'executor_reregistration_timeout.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp 927b9c3fbc3373b49c799da6e3f770b73964f9e5
>
>
> Diff: https://reviews.apache.org/r/59545/diff/5/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1 --gtest_break_on_failure`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 59545: Added a test to verify the agent flag
'executor_reregistration_timeout'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59545/#review177083
-----------------------------------------------------------
Also remove `this->` from the flag creation helpers if not necessary.
- Greg Mann
On May 30, 2017, 11:29 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59545/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 11:29 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-7540
> https://issues.apache.org/jira/browse/MESOS-7540
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a test to verify the correct behavior of
> the agent flag 'executor_reregistration_timeout.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp 927b9c3fbc3373b49c799da6e3f770b73964f9e5
>
>
> Diff: https://reviews.apache.org/r/59545/diff/5/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1 --gtest_break_on_failure`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 59545: Added a test to verify the agent flag
'executor_reregistration_timeout'.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59545/#review176403
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On May 30, 2017, 11:29 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59545/
> -----------------------------------------------------------
>
> (Updated May 30, 2017, 11:29 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-7540
> https://issues.apache.org/jira/browse/MESOS-7540
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a test to verify the correct behavior of
> the agent flag 'executor_reregistration_timeout.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp 927b9c3fbc3373b49c799da6e3f770b73964f9e5
>
>
> Diff: https://reviews.apache.org/r/59545/diff/5/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1 --gtest_break_on_failure`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 59545: Added a test to verify the agent flag
'executor_reregistration_timeout'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59545/
-----------------------------------------------------------
(Updated May 30, 2017, 11:29 p.m.)
Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
Changes
-------
Rebase.
Bugs: MESOS-7540
https://issues.apache.org/jira/browse/MESOS-7540
Repository: mesos
Description
-------
This patch adds a test to verify the correct behavior of
the agent flag 'executor_reregistration_timeout.
Diffs (updated)
-----
src/tests/slave_tests.cpp 927b9c3fbc3373b49c799da6e3f770b73964f9e5
Diff: https://reviews.apache.org/r/59545/diff/5/
Changes: https://reviews.apache.org/r/59545/diff/4-5/
Testing
-------
`bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1 --gtest_break_on_failure`
Thanks,
Greg Mann
Re: Review Request 59545: Added a test to verify the agent flag
'executor_reregistration_timeout'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59545/
-----------------------------------------------------------
(Updated May 27, 2017, 1:32 a.m.)
Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
Bugs: MESOS-7540
https://issues.apache.org/jira/browse/MESOS-7540
Repository: mesos
Description
-------
This patch adds a test to verify the correct behavior of
the agent flag 'executor_reregistration_timeout.
Diffs (updated)
-----
src/tests/slave_tests.cpp 927b9c3fbc3373b49c799da6e3f770b73964f9e5
Diff: https://reviews.apache.org/r/59545/diff/4/
Changes: https://reviews.apache.org/r/59545/diff/3-4/
Testing
-------
`bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1 --gtest_break_on_failure`
Thanks,
Greg Mann
Re: Review Request 59545: Added a test to verify the agent flag
'executor_reregistration_timeout'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59545/
-----------------------------------------------------------
(Updated May 25, 2017, 7:27 p.m.)
Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
Bugs: MESOS-7540
https://issues.apache.org/jira/browse/MESOS-7540
Repository: mesos
Description
-------
This patch adds a test to verify the correct behavior of
the agent flag 'executor_reregistration_timeout.
Diffs (updated)
-----
src/tests/slave_tests.cpp d124d594fb4f20db3f5dacbc7482d97161cca2df
Diff: https://reviews.apache.org/r/59545/diff/3/
Changes: https://reviews.apache.org/r/59545/diff/2-3/
Testing
-------
`bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1 --gtest_break_on_failure`
Thanks,
Greg Mann
Re: Review Request 59545: Added a test to verify the agent flag
'executor_reregistration_timeout'.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59545/#review176111
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/slave_tests.cpp
Line 7069 (original), 7069 (patched)
<https://reviews.apache.org/r/59545/#comment249438>
no need for this.
src/tests/slave_tests.cpp
Lines 7075 (patched)
<https://reviews.apache.org/r/59545/#comment249440>
s/ackCheckpoint/_statusUpdateAcknowledgement/
src/tests/slave_tests.cpp
Line 7077 (original), 7080 (patched)
<https://reviews.apache.org/r/59545/#comment249439>
no need for this.
src/tests/slave_tests.cpp
Lines 7084-7086 (original), 7088-7090 (patched)
<https://reviews.apache.org/r/59545/#comment249441>
no need for this.
src/tests/slave_tests.cpp
Line 7118 (original), 7125 (patched)
<https://reviews.apache.org/r/59545/#comment249442>
no need for this.
- Vinod Kone
On May 25, 2017, 6:01 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59545/
> -----------------------------------------------------------
>
> (Updated May 25, 2017, 6:01 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-7540
> https://issues.apache.org/jira/browse/MESOS-7540
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a test to verify the correct behavior of
> the agent flag 'executor_reregistration_timeout.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp d124d594fb4f20db3f5dacbc7482d97161cca2df
>
>
> Diff: https://reviews.apache.org/r/59545/diff/2/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1 --gtest_break_on_failure`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 59545: Added a test to verify the agent flag
'executor_reregistration_timeout'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59545/
-----------------------------------------------------------
(Updated May 25, 2017, 6:01 p.m.)
Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
Bugs: MESOS-7540
https://issues.apache.org/jira/browse/MESOS-7540
Repository: mesos
Description
-------
This patch adds a test to verify the correct behavior of
the agent flag 'executor_reregistration_timeout.
Diffs (updated)
-----
src/tests/slave_tests.cpp d124d594fb4f20db3f5dacbc7482d97161cca2df
Diff: https://reviews.apache.org/r/59545/diff/2/
Changes: https://reviews.apache.org/r/59545/diff/1-2/
Testing
-------
`bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1 --gtest_break_on_failure`
Thanks,
Greg Mann
Re: Review Request 59545: Added a test to verify the agent flag
'executor_reregistration_timeout'.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59545/#review176080
-----------------------------------------------------------
src/tests/slave_tests.cpp
Lines 7077 (patched)
<https://reviews.apache.org/r/59545/#comment249408>
You need to wait until the acknowledgement is checkpointed by the agent to ensure agent doesnt retry after a restart.
src/tests/slave_tests.cpp
Lines 7136 (patched)
<https://reviews.apache.org/r/59545/#comment249409>
what's the guarantee that master finished processing the reregisterslave message? need a clock settle?
- Vinod Kone
On May 24, 2017, 11:10 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59545/
> -----------------------------------------------------------
>
> (Updated May 24, 2017, 11:10 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-7540
> https://issues.apache.org/jira/browse/MESOS-7540
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a test to verify the correct behavior of
> the agent flag 'executor_reregistration_timeout.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp d124d594fb4f20db3f5dacbc7482d97161cca2df
>
>
> Diff: https://reviews.apache.org/r/59545/diff/1/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1 --gtest_break_on_failure`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 59545: Added a test to verify the agent flag
'executor_reregistration_timeout'.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59545/#review176073
-----------------------------------------------------------
Patch looks great!
Reviews applied: [59545]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 24, 2017, 11:10 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59545/
> -----------------------------------------------------------
>
> (Updated May 24, 2017, 11:10 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-7540
> https://issues.apache.org/jira/browse/MESOS-7540
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a test to verify the correct behavior of
> the agent flag 'executor_reregistration_timeout.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp d124d594fb4f20db3f5dacbc7482d97161cca2df
>
>
> Diff: https://reviews.apache.org/r/59545/diff/1/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1 --gtest_break_on_failure`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 59545: Added a test to verify the agent flag
'executor_reregistration_timeout'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59545/
-----------------------------------------------------------
(Updated May 24, 2017, 11:10 p.m.)
Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
Summary (updated)
-----------------
Added a test to verify the agent flag 'executor_reregistration_timeout'.
Bugs: MESOS-7540
https://issues.apache.org/jira/browse/MESOS-7540
Repository: mesos
Description
-------
This patch adds a test to verify the correct behavior of
the agent flag 'executor_reregistration_timeout.
Diffs
-----
src/tests/slave_tests.cpp d124d594fb4f20db3f5dacbc7482d97161cca2df
Diff: https://reviews.apache.org/r/59545/diff/1/
Testing (updated)
-------
`bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1 --gtest_break_on_failure`
Thanks,
Greg Mann