Re: Review Request 35755: Changed fetcher to handle leading whitespace in URLs.

[Benjamin Hindman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35755/#review92188
---

Ship it!


I'll take care of these minor issues and commit, thanks.


src/launcher/fetcher.cpp (line 158)


Why not log this after trimming the string so that the log prints out what 
we're actually going to be fetching?



src/tests/fetcher_tests.cpp 


As commented earlier, we keep two newlines between top-level definitions 
(here and the rest of the file).



src/tests/fetcher_tests.cpp (line 278)


While funky, this is really just a method declaration/definition which we 
put after constructor declarations/definitions.



src/tests/fetcher_tests.cpp (line 282)


We've tried to keep the name of the endpoint the same as the name of the 
method (i.e, not 'uri_test' and 'index'). In this case, I recommend just 'test'.


- Benjamin Hindman


On July 10, 2015, 5:19 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35755/
> ---
> 
> (Updated July 10, 2015, 5:19 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.

[Artem Harutyunyan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35755/
---

(Updated July 9, 2015, 10:19 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.

[Artem Harutyunyan]

> 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.

[Joris Van Remoortere]

> 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.

[Artem Harutyunyan]

---
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.

[Benjamin Hindman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35755/#review90423
---



src/launcher/fetcher.cpp (line 160)


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 


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)


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)


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
> 
>

Re: Review Request 35755: Changed fetcher to handle leading whitespace in URLs.

[Artem Harutyunyan]

---
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