You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gastón Kleiman <ga...@mesosphere.io> on 2018/09/12 20:53:10 UTC
Re: Review Request 68354: Added a test to verify agent authentication
retry backoff logic.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68354/#review208576
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/authentication_tests.cpp
Lines 422 (patched)
<https://reviews.apache.org/r/68354/#comment292636>
Nit: I am not a big fan of hard-coded magic numbers, so I'd declare `expected` as:
```
const std::vector<std::pair<Duration, Duration>> expectedLimits = {
{Seconds(5), Seconds(7)},
{Seconds(5), Seconds(9)},
{Seconds(5), Seconds(13)},
{Seconds(5), Seconds(21)},
{Seconds(5), Seconds(37)},
{Seconds(5), Minutes(1)},
{Seconds(5), Minutes(1)}
};
```
And then change the loop to:
```
for (auto it = expectedLimits.begin(); it != expectedLimits.end(); ++it) {
```
But I don't expect we to add more items to `expected`, so feel free to drop this issue if you prefer your current version.
- Gastón Kleiman
On Aug. 16, 2018, 4:54 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68354/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 4:54 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test verifies that the agent backs-off properly when
> retrying authentication according to the configured parameters.
>
> Also mocked `Slave::authenticate()` for this test.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 0420109ac93e1249906c52437e5859c5ee033fb6
> src/tests/authentication_tests.cpp f7a2cf17cf6154c9c67e405661bba57cf1254845
> src/tests/mock_slave.hpp 9a74bf35d2cab0a72ba6376392239d8080a49304
> src/tests/mock_slave.cpp 94a5b0d20475f49dde99108a009682b520175aa4
>
>
> Diff: https://reviews.apache.org/r/68354/diff/5/
>
>
> Testing
> -------
>
> make check
>
> Added test continuously running without failure.
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 68354: Added a test to verify agent authentication
retry backoff logic.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Sept. 12, 2018, 1:53 p.m., Gastón Kleiman wrote:
> > src/tests/authentication_tests.cpp
> > Lines 422 (patched)
> > <https://reviews.apache.org/r/68354/diff/5/?file=2074214#file2074214line422>
> >
> > Nit: I am not a big fan of hard-coded magic numbers, so I'd declare `expected` as:
> >
> > ```
> > const std::vector<std::pair<Duration, Duration>> expectedLimits = {
> > {Seconds(5), Seconds(7)},
> > {Seconds(5), Seconds(9)},
> > {Seconds(5), Seconds(13)},
> > {Seconds(5), Seconds(21)},
> > {Seconds(5), Seconds(37)},
> > {Seconds(5), Minutes(1)},
> > {Seconds(5), Minutes(1)}
> > };
> > ```
> >
> > And then change the loop to:
> >
> > ```
> > for (auto it = expectedLimits.begin(); it != expectedLimits.end(); ++it) {
> > ```
> >
> > But I don't expect we to add more items to `expected`, so feel free to drop this issue if you prefer your current version.
A good advice! The downside is it will complicate the loop a little bit (all right, I admit that I am just mumbling something to cover my laziness...)
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68354/#review208576
-----------------------------------------------------------
On Aug. 16, 2018, 4:54 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68354/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2018, 4:54 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test verifies that the agent backs-off properly when
> retrying authentication according to the configured parameters.
>
> Also mocked `Slave::authenticate()` for this test.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 0420109ac93e1249906c52437e5859c5ee033fb6
> src/tests/authentication_tests.cpp f7a2cf17cf6154c9c67e405661bba57cf1254845
> src/tests/mock_slave.hpp 9a74bf35d2cab0a72ba6376392239d8080a49304
> src/tests/mock_slave.cpp 94a5b0d20475f49dde99108a009682b520175aa4
>
>
> Diff: https://reviews.apache.org/r/68354/diff/5/
>
>
> Testing
> -------
>
> make check
>
> Added test continuously running without failure.
>
>
> Thanks,
>
> Meng Zhu
>
>