You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Artem Harutyunyan <ar...@mesosphere.io> on 2015/07/01 01:24:11 UTC
Re: Review Request 35755: Changed fetcher to handle leading
whitespace in URLs.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35755/
-----------------------------------------------------------
(Updated June 30, 2015, 4:24 p.m.)
Review request for mesos and Joris Van Remoortere.
Bugs: MESOS-2862
https://issues.apache.org/jira/browse/MESOS-2862
Repository: mesos
Description
-------
Fixes MESOS-2862
Diffs (updated)
-----
src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd
src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392
Diff: https://reviews.apache.org/r/35755/diff/
Testing
-------
- make check
- added a test case for fetcher
Thanks,
Artem Harutyunyan
Re: Review Request 35755: Changed fetcher to handle leading
whitespace in URLs.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35755/
-----------------------------------------------------------
(Updated July 6, 2015, 9:03 a.m.)
Review request for mesos and Joris Van Remoortere.
Bugs: MESOS-2862
https://issues.apache.org/jira/browse/MESOS-2862
Repository: mesos
Description
-------
Fixes MESOS-2862
Diffs
-----
src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd
src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392
Diff: https://reviews.apache.org/r/35755/diff/
Testing
-------
- make check
- added a test case for fetcher
Thanks,
Artem Harutyunyan
Re: Review Request 35755: Changed fetcher to handle leading
whitespace in URLs.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On July 5, 2015, 8:02 p.m., Benjamin Hindman wrote:
> >
>
> Joris Van Remoortere wrote:
> Hey Ben,
> Great questions in the latter half of your review.
> It turns out that the original test was malformed. It happened to pass because there is also a help process that responds to the `/help` endpoint.
> The original intent of the test, however, was to ensure that the newly added `/help` endpoint would work. That newly added enpoint was actually never being hit.
>
> The name is being passed through to the HTTPProcess constructor explicitly to make the url at which the test endpoint is being hit stable. If the default `Process` constructor is used, then we end up using the auto-incrementing ids which can be hard to deterministically hit.
>
> I agree that we should stick with a single endpoint as you suggested; however, we need to change the name to something unique to avoid this test from passing accidentally when the intended path is broken.
>
> We definitely need a comment as to why we're explicitly specifying the name of the process (as described above). I have seen a couple of people bang their heads against this, so let's add cleared documentation to the headers / doxygen as well!
Ben suggested to use MOCK_METHOD/EXPECT_CALL to make sure that the right callback is being invoked. As for the named processes, Ben suggested to use process.self().id instead of having named processes.
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35755/#review90423
-----------------------------------------------------------
On July 6, 2015, 9:03 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35755/
> -----------------------------------------------------------
>
> (Updated July 6, 2015, 9:03 a.m.)
>
>
> Review request for mesos and Joris Van Remoortere.
>
>
> Bugs: MESOS-2862
> https://issues.apache.org/jira/browse/MESOS-2862
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixes MESOS-2862
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd
> src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392
>
> Diff: https://reviews.apache.org/r/35755/diff/
>
>
> Testing
> -------
>
> - make check
> - added a test case for fetcher
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 35755: Changed fetcher to handle leading
whitespace in URLs.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On July 6, 2015, 3:02 a.m., Benjamin Hindman wrote:
> >
Hey Ben,
Great questions in the latter half of your review.
It turns out that the original test was malformed. It happened to pass because there is also a help process that responds to the `/help` endpoint.
The original intent of the test, however, was to ensure that the newly added `/help` endpoint would work. That newly added enpoint was actually never being hit.
The name is being passed through to the HTTPProcess constructor explicitly to make the url at which the test endpoint is being hit stable. If the default `Process` constructor is used, then we end up using the auto-incrementing ids which can be hard to deterministically hit.
I agree that we should stick with a single endpoint as you suggested; however, we need to change the name to something unique to avoid this test from passing accidentally when the intended path is broken.
We definitely need a comment as to why we're explicitly specifying the name of the process (as described above). I have seen a couple of people bang their heads against this, so let's add cleared documentation to the headers / doxygen as well!
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35755/#review90423
-----------------------------------------------------------
On July 6, 2015, 4:03 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35755/
> -----------------------------------------------------------
>
> (Updated July 6, 2015, 4:03 p.m.)
>
>
> Review request for mesos and Joris Van Remoortere.
>
>
> Bugs: MESOS-2862
> https://issues.apache.org/jira/browse/MESOS-2862
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixes MESOS-2862
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd
> src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392
>
> Diff: https://reviews.apache.org/r/35755/diff/
>
>
> Testing
> -------
>
> - make check
> - added a test case for fetcher
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 35755: Changed fetcher to handle leading
whitespace in URLs.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35755/#review90423
-----------------------------------------------------------
src/launcher/fetcher.cpp (line 160)
<https://reviews.apache.org/r/35755/#comment143465>
See https://reviews.apache.org/r/36189, I added a "mode" to 'strings::trim' so that you can use it just for prefix or suffix trims too! Thus:
string trimmedSourceUri = strings::trim(sourceUri, strings::PREFIX);
Also, a pattern which we've used in the code base quite a bit is to "replace" 'sourceUri' by naming the parameter '_sourceUri' and then doing:
string sourceUri = strings::trim(_sourceUri, strings::PREFIX);
Alternatively, we've taken 'sourceUri' the parameter by value and then just done:
sourceUri = strings::trim(sourceUri, strings::PREFIX);
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/35755/#comment143466>
We've used two newlines between top-level definitions to more easily contrast the definitions (here and the rest of this review please).
src/tests/fetcher_tests.cpp (line 281)
<https://reviews.apache.org/r/35755/#comment143468>
What's the benefit of adding the new route? It seems like the extra test can still use the existing route (unless I'm missing something) and I'd rather not add it and leave people trying to understand why it was added).
src/tests/fetcher_tests.cpp (line 292)
<https://reviews.apache.org/r/35755/#comment143467>
It's not clear to me what adding the process name does here?
Perhaps I'm missing something, but I'd rather not have people reading the test trying to understand why we do it here but not other places unless it's actually necessary, and if it is, perhaps it needs a comment because it's not obvious to me!
- Benjamin Hindman
On June 30, 2015, 11:24 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35755/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 11:24 p.m.)
>
>
> Review request for mesos and Joris Van Remoortere.
>
>
> Bugs: MESOS-2862
> https://issues.apache.org/jira/browse/MESOS-2862
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixes MESOS-2862
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 8aee4901ec1289f43b5fa6b830c3488815ec24cd
> src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392
>
> Diff: https://reviews.apache.org/r/35755/diff/
>
>
> Testing
> -------
>
> - make check
> - added a test case for fetcher
>
>
> Thanks,
>
> Artem Harutyunyan
>
>