You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Klaus Ma <kl...@cguru.net> on 2015/07/15 04:57:15 UTC

Review Request 36501: MESOS-3023

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

Review request for mesos.


Repository: mesos


Description
-------

Fix for MESOS-3023 (Factoring out the pattern for URL generation)


Diffs
-----

  src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392 
  src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 

Diff: https://reviews.apache.org/r/36501/diff/


Testing
-------

1. Build successfully in Linux
2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest


Thanks,

Klaus Ma

Re: Review Request 36501: MESOS-3023

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review91863
-----------------------------------------------------------


Patch looks great!

Reviews applied: [36501]

All tests passed.

- Mesos ReviewBot


On July 16, 2015, 4:41 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 16, 2015, 4:41 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
>   src/tests/utils.hpp f2eed2e 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by haosdent huang <ha...@gmail.com>.

> On July 17, 2015, 2:20 p.m., haosdent huang wrote:
> > Ship It!

3rdparty/libprocess/src/tests/http_tests.cpp have a unit test case "TEST(URLTest, Stringification)" show how to use URL and stringify it. Maybe you could use URL according that.


- haosdent


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


On July 17, 2015, 2:06 p.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 17, 2015, 2:06 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by haosdent huang <ha...@gmail.com>.

> On July 17, 2015, 2:20 p.m., haosdent huang wrote:
> > src/tests/fetcher_tests.cpp, line 22
> > <https://reviews.apache.org/r/36501/diff/4/?file=1014441#file1014441line22>
> >
> >     Need sort it lexicographically.
> >     
> >     ```
> >     #include <sstream>
> >     #include <string>
> 
> haosdent huang wrote:
>     ```
>     #include <sstream>
>     #include <string>
>     ```
>     Sorry for forgot type ```

No need sstream here.


- haosdent


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


On July 17, 2015, 4:13 p.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 17, 2015, 4:13 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by haosdent huang <ha...@gmail.com>.

> On July 17, 2015, 2:20 p.m., haosdent huang wrote:
> > src/tests/fetcher_tests.cpp, line 22
> > <https://reviews.apache.org/r/36501/diff/4/?file=1014441#file1014441line22>
> >
> >     Need sort it lexicographically.
> >     
> >     ```
> >     #include <sstream>
> >     #include <string>

```
#include <sstream>
#include <string>
```
Sorry for forgot type ```


- haosdent


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


On July 17, 2015, 2:06 p.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 17, 2015, 2:06 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review92082
-----------------------------------------------------------

Ship it!


Ship It!


src/tests/fetcher_tests.cpp (line 22)
<https://reviews.apache.org/r/36501/#comment146021>

    Need sort it lexicographically.
    
    ```
    #include <sstream>
    #include <string>



src/tests/fetcher_tests.cpp (line 319)
<https://reviews.apache.org/r/36501/#comment146022>

    Not need use urlStream and import sstream here because we have stringify.hpp. Just need
    
    ```
    stringify(url)
    ```


- haosdent huang


On July 17, 2015, 2:06 p.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 17, 2015, 2:06 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review92093
-----------------------------------------------------------


Patch looks great!

Reviews applied: [36501]

All tests passed.

- Mesos ReviewBot


On July 17, 2015, 2:06 p.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 17, 2015, 2:06 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.

> On 七月 17, 2015, 5:39 p.m., haosdent huang wrote:
> > src/tests/fetcher_tests.cpp, line 66
> > <https://reviews.apache.org/r/36501/diff/5/?file=1014595#file1014595line66>
> >
> >     Because use stringify, could remove it now.

yes. agree. i'll update the code.


- Klaus


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


On 七月 17, 2015, 4:13 p.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated 七月 17, 2015, 4:13 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review92108
-----------------------------------------------------------



src/tests/fetcher_tests.cpp (line 22)
<https://reviews.apache.org/r/36501/#comment146048>

    Because use stringify, could remove it now.



src/tests/fetcher_tests.cpp (line 66)
<https://reviews.apache.org/r/36501/#comment146047>

    Because use stringify, could remove it now.


- haosdent huang


On July 17, 2015, 4:13 p.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 17, 2015, 4:13 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.

> On July 20, 2015, 4:42 p.m., haosdent huang wrote:
> > src/tests/fetcher_tests.cpp, line 297
> > <https://reviews.apache.org/r/36501/diff/6/?file=1015170#file1015170line297>
> >
> >     According https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md#function-definitioninvocation You indent is not correct here. Maybe need change to like this
> >     
> >     ```
> >     process::http::URL url(
> >         "http",
> >         process.self().address.ip,
> >         process.self().address.port,
> >         "/help");
> >     ```
> >     
> >     But I perfer chang it like this
> >     ```
> >     const network::Address& address = process.self().address;
> >     process::http::URL url("http", address.ip, address.port, "/help");
> >     ```
> >     
> >     Or add `using URL` like this
> >     ```
> >     using process::Future;
> >     
> >     using process::http::URL; (Left a blank below and after process::Future)
> >     ```
> >     
> >     and then
> >     ```
> >     const network::Address& address = process.self().address;
> >     URL url("http", address.ip, address.port, "/help");
> >     ```
> 
> Bernd Mathiske wrote:
>     I agree. Thanks, haosdent!

I address the comments by option 2. Reguarding "using URL", it'll compile because there's also mesos::URL here. But mesos::URL is a protobuf class, it will make code more complex.


- Klaus


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


On July 18, 2015, 9:47 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 18, 2015, 9:47 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.

> On July 20, 2015, 4:42 p.m., haosdent huang wrote:
> >
> 
> haosdent huang wrote:
>     Its a bit difficult to follow the mesos style guide at first. Maybe the committer could help you reformat it when summit @klausma1982 . :-)

Thanks very much for your patience; yes, it tooks time for me to swith between different code style :).


> On July 20, 2015, 4:42 p.m., haosdent huang wrote:
> > src/tests/fetcher_tests.cpp, line 314
> > <https://reviews.apache.org/r/36501/diff/6/?file=1015170#file1015170line314>
> >
> >     It would be better to add ```#include <stout/stringify.hpp>``` after ```#include <stout/protobuf.hpp>```

stout/stringify.hpp has been included; so did not add it.


- Klaus


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


On July 18, 2015, 9:47 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 18, 2015, 9:47 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by haosdent huang <ha...@gmail.com>.

> On July 20, 2015, 4:42 p.m., haosdent huang wrote:
> >

Its a bit difficult to follow the mesos style guide at first. Maybe the committer could help you reformat it when summit @klausma1982 . :-)


- haosdent


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


On July 18, 2015, 9:47 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 18, 2015, 9:47 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Bernd Mathiske <be...@mesosphere.io>.

> On July 20, 2015, 9:42 a.m., haosdent huang wrote:
> > src/tests/fetcher_tests.cpp, line 297
> > <https://reviews.apache.org/r/36501/diff/6/?file=1015170#file1015170line297>
> >
> >     According https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md#function-definitioninvocation You indent is not correct here. Maybe need change to like this
> >     
> >     ```
> >     process::http::URL url(
> >         "http",
> >         process.self().address.ip,
> >         process.self().address.port,
> >         "/help");
> >     ```
> >     
> >     But I perfer chang it like this
> >     ```
> >     const network::Address& address = process.self().address;
> >     process::http::URL url("http", address.ip, address.port, "/help");
> >     ```
> >     
> >     Or add `using URL` like this
> >     ```
> >     using process::Future;
> >     
> >     using process::http::URL; (Left a blank below and after process::Future)
> >     ```
> >     
> >     and then
> >     ```
> >     const network::Address& address = process.self().address;
> >     URL url("http", address.ip, address.port, "/help");
> >     ```

I agree. Thanks, haosdent!


- Bernd


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


On July 18, 2015, 2:47 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 18, 2015, 2:47 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Bernd Mathiske <be...@mesosphere.io>.

> On July 20, 2015, 9:42 a.m., haosdent huang wrote:
> >
> 
> haosdent huang wrote:
>     Its a bit difficult to follow the mesos style guide at first. Maybe the committer could help you reformat it when summit @klausma1982 . :-)
> 
> Klaus Ma wrote:
>     Thanks very much for your patience; yes, it tooks time for me to swith between different code style :).

Me too!


> On July 20, 2015, 9:42 a.m., haosdent huang wrote:
> > src/tests/fetcher_tests.cpp, line 314
> > <https://reviews.apache.org/r/36501/diff/6/?file=1015170#file1015170line314>
> >
> >     It would be better to add ```#include <stout/stringify.hpp>``` after ```#include <stout/protobuf.hpp>```
> 
> Klaus Ma wrote:
>     stout/stringify.hpp has been included; so did not add it.

No matter. Best to be explicit even if redundant, IMHO. (That's why we have provisions in headers that they only get processed once.)


- Bernd


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


On July 24, 2015, 2:57 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 24, 2015, 2:57 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp e2a52f7 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review92270
-----------------------------------------------------------



src/tests/fetcher_tests.cpp (line 297)
<https://reviews.apache.org/r/36501/#comment146333>

    According https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md#function-definitioninvocation You indent is not correct here. Maybe need change to like this
    
    ```
    process::http::URL url(
        "http",
        process.self().address.ip,
        process.self().address.port,
        "/help");
    ```
    
    But I perfer chang it like this
    ```
    const network::Address& address = process.self().address;
    process::http::URL url("http", address.ip, address.port, "/help");
    ```
    
    Or add `using URL` like this
    ```
    using process::Future;
    
    using process::http::URL; (Left a blank below and after process::Future)
    ```
    
    and then
    ```
    const network::Address& address = process.self().address;
    URL url("http", address.ip, address.port, "/help");
    ```



src/tests/fetcher_tests.cpp (line 314)
<https://reviews.apache.org/r/36501/#comment146334>

    It would be better to add ```#include <stout/stringify.hpp>``` after ```#include <stout/protobuf.hpp>```


- haosdent huang


On July 18, 2015, 9:47 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 18, 2015, 9:47 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review92170
-----------------------------------------------------------


Patch looks great!

Reviews applied: [36501]

All tests passed.

- Mesos ReviewBot


On July 18, 2015, 9:47 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 18, 2015, 9:47 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review92889
-----------------------------------------------------------


Patch looks great!

Reviews applied: [36501]

All tests passed.

- Mesos ReviewBot


On July 24, 2015, 9:57 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 24, 2015, 9:57 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp e2a52f7 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.

On July 24, 2015, 2:01 p.m., Klaus Ma wrote:
> > If you don't get to them first, I will fix the remaining little style suggestions when committing.

OK, please help to fix it. I'll learn it from the final code :). Thanks very much.


- Klaus


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


On July 24, 2015, 9:57 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 24, 2015, 9:57 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp e2a52f7 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review92911
-----------------------------------------------------------

Ship it!



src/tests/fetcher_tests.cpp (line 295)
<https://reviews.apache.org/r/36501/#comment147177>

    It would be slightly better to separate these declarations more obviously by putting a new line here (between line 294 and 295).


If you don't get to them first, I will fix the remaining little style suggestions when committing.

- Bernd Mathiske


On July 24, 2015, 2:57 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 24, 2015, 2:57 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp e2a52f7 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/
-----------------------------------------------------------

(Updated July 24, 2015, 9:57 a.m.)


Review request for mesos.


Bugs: MESOS-3023
    https://issues.apache.org/jira/browse/MESOS-3023


Repository: mesos


Description
-------

Fix for MESOS-3023 (Factoring out the pattern for URL generation)


Diffs (updated)
-----

  src/tests/fetcher_tests.cpp e2a52f7 

Diff: https://reviews.apache.org/r/36501/diff/


Testing
-------

1. Build successfully in Linux
2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest


Thanks,

Klaus Ma

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/
-----------------------------------------------------------

(Updated July 18, 2015, 9:47 a.m.)


Review request for mesos.


Bugs: MESOS-3023
    https://issues.apache.org/jira/browse/MESOS-3023


Repository: mesos


Description
-------

Fix for MESOS-3023 (Factoring out the pattern for URL generation)


Diffs (updated)
-----

  src/tests/fetcher_tests.cpp ae10c42 

Diff: https://reviews.apache.org/r/36501/diff/


Testing
-------

1. Build successfully in Linux
2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest


Thanks,

Klaus Ma

Re: Review Request 36501: MESOS-3023

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review92104
-----------------------------------------------------------


Patch looks great!

Reviews applied: [36501]

All tests passed.

- Mesos ReviewBot


On July 17, 2015, 4:13 p.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 17, 2015, 4:13 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/
-----------------------------------------------------------

(Updated July 17, 2015, 4:13 p.m.)


Review request for mesos.


Bugs: MESOS-3023
    https://issues.apache.org/jira/browse/MESOS-3023


Repository: mesos


Description
-------

Fix for MESOS-3023 (Factoring out the pattern for URL generation)


Diffs (updated)
-----

  src/tests/fetcher_tests.cpp ae10c42 

Diff: https://reviews.apache.org/r/36501/diff/


Testing
-------

1. Build successfully in Linux
2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest


Thanks,

Klaus Ma

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/
-----------------------------------------------------------

(Updated July 17, 2015, 2:06 p.m.)


Review request for mesos.


Bugs: MESOS-3023
    https://issues.apache.org/jira/browse/MESOS-3023


Repository: mesos


Description
-------

Fix for MESOS-3023 (Factoring out the pattern for URL generation)


Diffs (updated)
-----

  src/tests/fetcher_tests.cpp ae10c42 

Diff: https://reviews.apache.org/r/36501/diff/


Testing
-------

1. Build successfully in Linux
2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest


Thanks,

Klaus Ma

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.

> On July 17, 2015, 7:13 a.m., haosdent huang wrote:
> > src/tests/utils.hpp, line 72
> > <https://reviews.apache.org/r/36501/diff/3/?file=1013117#file1013117line72>
> >
> >     How about check path length and use `path[0]` here? Instead of `*(path.begin())`

I think we can use startsWith to check it.


> On July 17, 2015, 7:13 a.m., haosdent huang wrote:
> > src/tests/utils.hpp, line 69
> > <https://reviews.apache.org/r/36501/diff/3/?file=1013117#file1013117line69>
> >
> >     Indent also not correct here. And if `net::getHostname(process.self().address.ip)` return error, does it make incorrect url?

Yes, agree with that. If we want to build a new function, Try<std::string> should be the type of return value.


> On July 17, 2015, 7:13 a.m., haosdent huang wrote:
> > src/tests/utils.hpp, line 55
> > <https://reviews.apache.org/r/36501/diff/3/?file=1013117#file1013117line55>
> >
> >     Because we use "We use lowerCamelCase for function names (Google uses mixed case for regular functions; and their accessors and mutators match the name of the variable)."
> >     
> >     So maybe need to change it like this:
> >     
> >     ```
> >     template <class T>
> >     std::string endpointUrl(
> >         process::Process<T>& process,
> >         const std::string& path,
> >         ....)
> >     ```
> >     The indent also break the style guild rule here.

Agree.


> On July 17, 2015, 7:13 a.m., haosdent huang wrote:
> > src/tests/utils.hpp, line 60
> > <https://reviews.apache.org/r/36501/diff/3/?file=1013117#file1013117line60>
> >
> >     I still suggest use <string> method and remove <cstring>

Yes; if we can remove it because of endsWith util fuction.


> On July 17, 2015, 7:13 a.m., haosdent huang wrote:
> > src/tests/utils.hpp, line 64
> > <https://reviews.apache.org/r/36501/diff/3/?file=1013117#file1013117line64>
> >
> >     We have endsWith method in strings.hpp. Maybe use could use it. Also the style looks not correct here. Maybe need chang it like this:
> >     ```
> >     if (protocal.length() <= len ||
> >     ```

Thanks; just found endsWith in our source code.


- Klaus


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


On July 17, 2015, 2:06 p.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 17, 2015, 2:06 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review92033
-----------------------------------------------------------



src/tests/utils.hpp (line 30)
<https://reviews.apache.org/r/36501/#comment145909>

    Need from the style guide:
    ```
    
    #include <process/process.hpp>
    
    #include <stout/json.hpp>
    #include <stout/option.hpp>
    ```



src/tests/utils.hpp (line 55)
<https://reviews.apache.org/r/36501/#comment145910>

    Because we use "We use lowerCamelCase for function names (Google uses mixed case for regular functions; and their accessors and mutators match the name of the variable)."
    
    So maybe need to change it like this:
    
    ```
    template <class T>
    std::string endpointUrl(
        process::Process<T>& process,
        const std::string& path,
        ....)
    ```
    The indent also break the style guild rule here.



src/tests/utils.hpp (line 60)
<https://reviews.apache.org/r/36501/#comment145911>

    I still suggest use <string> method and remove <cstring>



src/tests/utils.hpp (line 64)
<https://reviews.apache.org/r/36501/#comment145912>

    We have endsWith method in strings.hpp. Maybe use could use it. Also the style looks not correct here. Maybe need chang it like this:
    ```
    if (protocal.length() <= len ||
    ```



src/tests/utils.hpp (line 69)
<https://reviews.apache.org/r/36501/#comment145913>

    Indent also not correct here. And if `net::getHostname(process.self().address.ip)` return error, does it make incorrect url?



src/tests/utils.hpp (line 72)
<https://reviews.apache.org/r/36501/#comment145914>

    How about check path length and use `path[0]` here? Instead of `*(path.begin())`


- haosdent huang


On July 16, 2015, 5:47 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 16, 2015, 5:47 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
>   src/tests/utils.hpp f2eed2e 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review91871
-----------------------------------------------------------


Patch looks great!

Reviews applied: [36501]

All tests passed.

- Mesos ReviewBot


On July 16, 2015, 5:47 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 16, 2015, 5:47 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
>   src/tests/utils.hpp f2eed2e 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/
-----------------------------------------------------------

(Updated July 16, 2015, 5:47 a.m.)


Review request for mesos.


Bugs: MESOS-3023
    https://issues.apache.org/jira/browse/MESOS-3023


Repository: mesos


Description
-------

Fix for MESOS-3023 (Factoring out the pattern for URL generation)


Diffs (updated)
-----

  src/tests/fetcher_tests.cpp ae10c42 
  src/tests/utils.hpp f2eed2e 

Diff: https://reviews.apache.org/r/36501/diff/


Testing
-------

1. Build successfully in Linux
2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest


Thanks,

Klaus Ma

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/
-----------------------------------------------------------

(Updated July 16, 2015, 4:41 a.m.)


Review request for mesos.


Bugs: MESOS-3023
    https://issues.apache.org/jira/browse/MESOS-3023


Repository: mesos


Description
-------

Fix for MESOS-3023 (Factoring out the pattern for URL generation)


Diffs (updated)
-----

  src/tests/fetcher_tests.cpp ae10c42 
  src/tests/utils.hpp f2eed2e 

Diff: https://reviews.apache.org/r/36501/diff/


Testing
-------

1. Build successfully in Linux
2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest


Thanks,

Klaus Ma

Re: Review Request 36501: MESOS-3023

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review91707
-----------------------------------------------------------


Patch looks great!

Reviews applied: [36501]

All tests passed.

- Mesos ReviewBot


On July 15, 2015, 2:59 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 15, 2015, 2:59 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392 
>   src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.

> On July 15, 2015, 9:19 p.m., Joseph Wu wrote:
> > src/tests/utils.hpp, line 55
> > <https://reviews.apache.org/r/36501/diff/1/?file=1012134#file1012134line55>
> >
> >     * Tab size = 2 spaces.
> >     * Parameters are indented by 4 spaces.
> >     * Comments start with a capital letter and end with a period.
> >     * Logical blocks have the opening "{" on the same line.

Thanks very much for your input :). The code diff has been updated.


- Klaus


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


On July 16, 2015, 4:41 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 16, 2015, 4:41 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
>   src/tests/utils.hpp f2eed2e 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review91823
-----------------------------------------------------------



src/tests/utils.hpp (line 55)
<https://reviews.apache.org/r/36501/#comment145460>

    * Tab size = 2 spaces.
    * Parameters are indented by 4 spaces.
    * Comments start with a capital letter and end with a period.
    * Logical blocks have the opening "{" on the same line.


- Joseph Wu


On July 14, 2015, 7:59 p.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 14, 2015, 7:59 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392 
>   src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review91732
-----------------------------------------------------------



src/tests/utils.hpp (line 26)
<https://reviews.apache.org/r/36501/#comment145331>

    Using ::strlen to get the length of "://", did not want to hardcord to 3.



src/tests/utils.hpp (line 54)
<https://reviews.apache.org/r/36501/#comment145334>

    As far as I known, C++ can not declare template in header file and implete it in cpp. I also have a try with the example.
    
    In C++11, it only introduced extern template to avoid duplicated template instance.



src/tests/utils.hpp (line 55)
<https://reviews.apache.org/r/36501/#comment145335>

    Do you mean ident of parameters?


- Klaus Ma


On July 15, 2015, 2:59 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 15, 2015, 2:59 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392 
>   src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.

> On July 15, 2015, 4:16 a.m., haosdent huang wrote:
> > src/tests/utils.hpp, line 54
> > <https://reviews.apache.org/r/36501/diff/1/?file=1012134#file1012134line54>
> >
> >     I suggest move the implement to cpp file.

As far as I known, C++ can not declare template in header file and implete it in cpp. I also have a try with the example.

In C++11, it only introduced extern template to avoid duplicated template instance.


- Klaus


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


On July 16, 2015, 5:47 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 16, 2015, 5:47 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
>   src/tests/utils.hpp f2eed2e 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.

> On July 15, 2015, 4:16 a.m., haosdent huang wrote:
> > src/tests/utils.hpp, line 55
> > <https://reviews.apache.org/r/36501/diff/1/?file=1012134#file1012134line55>
> >
> >     The code styple need change to follow https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md

I've read this code style guidance and updated code accordingly :).
Thanks very much for your comments.


- Klaus


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


On July 16, 2015, 5:47 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 16, 2015, 5:47 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c42 
>   src/tests/utils.hpp f2eed2e 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review91706
-----------------------------------------------------------



src/tests/utils.hpp (line 26)
<https://reviews.apache.org/r/36501/#comment145266>

    why not use methods in <string>?



src/tests/utils.hpp (line 54)
<https://reviews.apache.org/r/36501/#comment145265>

    I suggest move the implement to cpp file.



src/tests/utils.hpp (line 55)
<https://reviews.apache.org/r/36501/#comment145267>

    The code styple need change to follow https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md



src/tests/utils.hpp (line 56)
<https://reviews.apache.org/r/36501/#comment145268>

    


- haosdent huang


On July 15, 2015, 2:59 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36501/
> -----------------------------------------------------------
> 
> (Updated July 15, 2015, 2:59 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3023
>     https://issues.apache.org/jira/browse/MESOS-3023
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix for MESOS-3023 (Factoring out the pattern for URL generation)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392 
>   src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 
> 
> Diff: https://reviews.apache.org/r/36501/diff/
> 
> 
> Testing
> -------
> 
> 1. Build successfully in Linux
> 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 36501: MESOS-3023

Posted by Klaus Ma <kl...@cguru.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/
-----------------------------------------------------------

(Updated July 15, 2015, 2:59 a.m.)


Review request for mesos.


Bugs: MESOS-3023
    https://issues.apache.org/jira/browse/MESOS-3023


Repository: mesos


Description
-------

Fix for MESOS-3023 (Factoring out the pattern for URL generation)


Diffs
-----

  src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392 
  src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 

Diff: https://reviews.apache.org/r/36501/diff/


Testing
-------

1. Build successfully in Linux
2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest


Thanks,

Klaus Ma