You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Timothy Chen <tn...@apache.org> on 2015/12/25 09:10:13 UTC
Review Request 41715: Support parsing url in libprocess.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41715/
-----------------------------------------------------------
Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
Repository: mesos
Description
-------
Support parsing url in libprocess.
Diffs
-----
3rdparty/libprocess/include/process/http.hpp f0666f0fa48c4f3a98332d12066561a02a715236
3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969
3rdparty/libprocess/src/tests/http_tests.cpp 19261502be220aaa40add7ce30a9b2b65d1d9fdc
Diff: https://reviews.apache.org/r/41715/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 41715: Support parsing url in libprocess.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41715/#review111903
-----------------------------------------------------------
Ship it!
Verified that the parse method handles some conner cases well. :)
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1204 - 1207)
<https://reviews.apache.org/r/41715/#comment172198>
Should we use `EXPECT_` here?
It does not impact on this test result, but we may not expect each of them to yield a fatal failure. (Understand that the test passed though, but it can become an example to someone else, and we do want tests continue to run).
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1211 - 1214)
<https://reviews.apache.org/r/41715/#comment172201>
Ditto.
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1218 - 1221)
<https://reviews.apache.org/r/41715/#comment172200>
Ditto.
- Gilbert Song
On Dec. 25, 2015, 12:10 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41715/
> -----------------------------------------------------------
>
> (Updated Dec. 25, 2015, 12:10 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing url in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp f0666f0fa48c4f3a98332d12066561a02a715236
> 3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969
> 3rdparty/libprocess/src/tests/http_tests.cpp 19261502be220aaa40add7ce30a9b2b65d1d9fdc
>
> Diff: https://reviews.apache.org/r/41715/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 41715: Support parsing url in libprocess.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41715/#review111887
-----------------------------------------------------------
Patch looks great!
Reviews applied: [41715]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 25, 2015, 8:10 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41715/
> -----------------------------------------------------------
>
> (Updated Dec. 25, 2015, 8:10 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing url in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp f0666f0fa48c4f3a98332d12066561a02a715236
> 3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969
> 3rdparty/libprocess/src/tests/http_tests.cpp 19261502be220aaa40add7ce30a9b2b65d1d9fdc
>
> Diff: https://reviews.apache.org/r/41715/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 41715: Support parsing url in libprocess.
Posted by Timothy Chen <tn...@apache.org>.
> On Dec. 28, 2015, 8:02 p.m., Jojy Varghese wrote:
> > 3rdparty/libprocess/src/http.cpp, line 183
> > <https://reviews.apache.org/r/41715/diff/2/?file=1176450#file1176450line183>
> >
> > Have we considered using a regex parser for doing this? C++11 regex support is added since gcc 4.9 and has been in clang for sometime now.
We can do this later, I took the toURL method you did in the registry client, made it more general moved it into libprocess for now.
I can add a TODO later for making it better.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41715/#review112061
-----------------------------------------------------------
On Dec. 26, 2015, 8:24 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41715/
> -----------------------------------------------------------
>
> (Updated Dec. 26, 2015, 8:24 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing url in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp f0666f0fa48c4f3a98332d12066561a02a715236
> 3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969
> 3rdparty/libprocess/src/tests/http_tests.cpp 19261502be220aaa40add7ce30a9b2b65d1d9fdc
>
> Diff: https://reviews.apache.org/r/41715/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 41715: Support parsing url in libprocess.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41715/#review112061
-----------------------------------------------------------
3rdparty/libprocess/src/http.cpp (line 183)
<https://reviews.apache.org/r/41715/#comment172369>
Have we considered using a regex parser for doing this? C++11 regex support is added since gcc 4.9 and has been in clang for sometime now.
- Jojy Varghese
On Dec. 26, 2015, 8:24 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41715/
> -----------------------------------------------------------
>
> (Updated Dec. 26, 2015, 8:24 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing url in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp f0666f0fa48c4f3a98332d12066561a02a715236
> 3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969
> 3rdparty/libprocess/src/tests/http_tests.cpp 19261502be220aaa40add7ce30a9b2b65d1d9fdc
>
> Diff: https://reviews.apache.org/r/41715/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 41715: Support parsing url in libprocess.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41715/#review112328
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/http.cpp (line 169)
<https://reviews.apache.org/r/41715/#comment172790>
static please since this is local to the file.
3rdparty/libprocess/src/http.cpp (line 212)
<https://reviews.apache.org/r/41715/#comment172791>
s/addressParts/tokens/
- Jie Yu
On Dec. 28, 2015, 8:23 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41715/
> -----------------------------------------------------------
>
> (Updated Dec. 28, 2015, 8:23 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing url in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp f0666f0fa48c4f3a98332d12066561a02a715236
> 3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969
> 3rdparty/libprocess/src/tests/http_tests.cpp 19261502be220aaa40add7ce30a9b2b65d1d9fdc
>
> Diff: https://reviews.apache.org/r/41715/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 41715: Support parsing url in libprocess.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41715/
-----------------------------------------------------------
(Updated Dec. 28, 2015, 8:23 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
Repository: mesos
Description
-------
Support parsing url in libprocess.
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp f0666f0fa48c4f3a98332d12066561a02a715236
3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969
3rdparty/libprocess/src/tests/http_tests.cpp 19261502be220aaa40add7ce30a9b2b65d1d9fdc
Diff: https://reviews.apache.org/r/41715/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 41715: Support parsing url in libprocess.
Posted by Timothy Chen <tn...@apache.org>.
> On Dec. 27, 2015, 2:56 a.m., Jojy Varghese wrote:
> > 3rdparty/libprocess/src/http.cpp, line 182
> > <https://reviews.apache.org/r/41715/diff/2/?file=1176450#file1176450line182>
> >
> > I would add some comments through the function to explain each sub-section's intent.
We usually try not to comment on everything unless it's confusing or not easy to understand. Which parts you feel like we need to comment on if it's not easy to understand?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41715/#review111918
-----------------------------------------------------------
On Dec. 26, 2015, 8:24 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41715/
> -----------------------------------------------------------
>
> (Updated Dec. 26, 2015, 8:24 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing url in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp f0666f0fa48c4f3a98332d12066561a02a715236
> 3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969
> 3rdparty/libprocess/src/tests/http_tests.cpp 19261502be220aaa40add7ce30a9b2b65d1d9fdc
>
> Diff: https://reviews.apache.org/r/41715/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 41715: Support parsing url in libprocess.
Posted by Timothy Chen <tn...@apache.org>.
> On Dec. 27, 2015, 2:56 a.m., Jojy Varghese wrote:
> > 3rdparty/libprocess/src/http.cpp, line 190
> > <https://reviews.apache.org/r/41715/diff/2/?file=1176450#file1176450line190>
> >
> > Can we avoid using magic numbers ('3' here)?
It's paired with the magic string '://' :) I'm not sure what I would even call this if we move it out.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41715/#review111918
-----------------------------------------------------------
On Dec. 26, 2015, 8:24 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41715/
> -----------------------------------------------------------
>
> (Updated Dec. 26, 2015, 8:24 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing url in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp f0666f0fa48c4f3a98332d12066561a02a715236
> 3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969
> 3rdparty/libprocess/src/tests/http_tests.cpp 19261502be220aaa40add7ce30a9b2b65d1d9fdc
>
> Diff: https://reviews.apache.org/r/41715/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 41715: Support parsing url in libprocess.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41715/#review111918
-----------------------------------------------------------
3rdparty/libprocess/src/http.cpp (line 182)
<https://reviews.apache.org/r/41715/#comment172204>
I would add some comments through the function to explain each sub-section's intent.
3rdparty/libprocess/src/http.cpp (line 190)
<https://reviews.apache.org/r/41715/#comment172205>
Can we avoid using magic numbers ('3' here)?
3rdparty/libprocess/src/http.cpp (line 205)
<https://reviews.apache.org/r/41715/#comment172206>
s/Domain/Host as it could be a IP address also?
- Jojy Varghese
On Dec. 26, 2015, 8:24 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41715/
> -----------------------------------------------------------
>
> (Updated Dec. 26, 2015, 8:24 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing url in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp f0666f0fa48c4f3a98332d12066561a02a715236
> 3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969
> 3rdparty/libprocess/src/tests/http_tests.cpp 19261502be220aaa40add7ce30a9b2b65d1d9fdc
>
> Diff: https://reviews.apache.org/r/41715/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 41715: Support parsing url in libprocess.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41715/
-----------------------------------------------------------
(Updated Dec. 26, 2015, 8:24 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
Repository: mesos
Description
-------
Support parsing url in libprocess.
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp f0666f0fa48c4f3a98332d12066561a02a715236
3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969
3rdparty/libprocess/src/tests/http_tests.cpp 19261502be220aaa40add7ce30a9b2b65d1d9fdc
Diff: https://reviews.apache.org/r/41715/diff/
Testing
-------
make check
Thanks,
Timothy Chen