You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Bernd Mathiske <be...@mesosphere.io> on 2016/01/13 11:07:17 UTC
Review Request 42246: Fixed support for non-HDFS URIs by Hadoop
client.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42246/
-----------------------------------------------------------
Review request for mesos, haosdent huang, Jie Yu, and switched to 'mcypark'.
Bugs: MESOS-4304
https://issues.apache.org/jira/browse/MESOS-4304
Repository: mesos
Description
-------
Fixed support for non-HDFS URIs by Hadoop client.
Diffs
-----
src/hdfs/hdfs.cpp 51f016b049c6e947c4d27fbbbf79c53f9ec5a51e
Diff: https://reviews.apache.org/r/42246/diff/
Testing
-------
make check
Thanks,
Bernd Mathiske
Re: Review Request 42246: Fixed support for non-HDFS URIs by Hadoop
client.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42246/#review114228
-----------------------------------------------------------
src/hdfs/hdfs.cpp (line 315)
<https://reviews.apache.org/r/42246/#comment175043>
According the document here https://docs.google.com/document/d/1p8tmQrGtxG6keZVo19gvHPr9WHxeny6PHTVofcLWqco/edit# We could have below cases in uri.
```
// mailto:John.Doe@example.com
// news:comp.infosystems.www.servers.unix
// tel:+1-816-555-1212
// telnet://192.0.2.16:80/
```
But as I know, all protocol supported by HDFS client contains `://`, here also look good to me.
- haosdent huang
On Jan. 13, 2016, 10:07 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42246/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 10:07 a.m.)
>
>
> Review request for mesos, haosdent huang, Jie Yu, and switched to 'mcypark'.
>
>
> Bugs: MESOS-4304
> https://issues.apache.org/jira/browse/MESOS-4304
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed support for non-HDFS URIs by Hadoop client.
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.cpp 51f016b049c6e947c4d27fbbbf79c53f9ec5a51e
>
> Diff: https://reviews.apache.org/r/42246/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 42246: Fixed support for non-HDFS URIs by Hadoop
client.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42246/#review114173
-----------------------------------------------------------
Ship it!
Ship It!
- haosdent huang
On Jan. 13, 2016, 10:07 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42246/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 10:07 a.m.)
>
>
> Review request for mesos, haosdent huang, Jie Yu, and switched to 'mcypark'.
>
>
> Bugs: MESOS-4304
> https://issues.apache.org/jira/browse/MESOS-4304
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed support for non-HDFS URIs by Hadoop client.
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.cpp 51f016b049c6e947c4d27fbbbf79c53f9ec5a51e
>
> Diff: https://reviews.apache.org/r/42246/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 42246: Fixed support for non-HDFS URIs by Hadoop
client.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42246/#review114716
-----------------------------------------------------------
src/hdfs/hdfs.cpp (line 313)
<https://reviews.apache.org/r/42246/#comment175582>
I guess you need to update the comment in hdfs.hpp
```// Normalize an HDFS path such that it is either an absolute path or
// a full hdfs:// URL.```
src/hdfs/hdfs.cpp (line 315)
<https://reviews.apache.org/r/42246/#comment175581>
What do you mean by misshaped? Mistyped?
- Joerg Schad
On Jan. 13, 2016, 10:07 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42246/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 10:07 a.m.)
>
>
> Review request for mesos, haosdent huang, Jie Yu, and switched to 'mcypark'.
>
>
> Bugs: MESOS-4304
> https://issues.apache.org/jira/browse/MESOS-4304
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed support for non-HDFS URIs by Hadoop client.
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.cpp 51f016b049c6e947c4d27fbbbf79c53f9ec5a51e
>
> Diff: https://reviews.apache.org/r/42246/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 42246: Fixed support for non-HDFS URIs by Hadoop
client.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42246/#review114720
-----------------------------------------------------------
Ship it!
(after my previous issues are resolved). In addition it would be great to great to have a test here, but as discussed it seems quite complex (and therefore unproportional) to test.
- Joerg Schad
On Jan. 13, 2016, 10:07 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42246/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 10:07 a.m.)
>
>
> Review request for mesos, haosdent huang, Jie Yu, and switched to 'mcypark'.
>
>
> Bugs: MESOS-4304
> https://issues.apache.org/jira/browse/MESOS-4304
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed support for non-HDFS URIs by Hadoop client.
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.cpp 51f016b049c6e947c4d27fbbbf79c53f9ec5a51e
>
> Diff: https://reviews.apache.org/r/42246/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 42246: Fixed support for non-HDFS URIs by Hadoop
client.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42246/#review115008
-----------------------------------------------------------
Patch looks great!
Reviews applied: [42246]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 18, 2016, 11:20 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42246/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2016, 11:20 a.m.)
>
>
> Review request for mesos, haosdent huang, Jie Yu, and switched to 'mcypark'.
>
>
> Bugs: MESOS-4304
> https://issues.apache.org/jira/browse/MESOS-4304
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Review: https://reviews.apache.org/r/42246/
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.hpp abdb9b9013abc5e7c2e0d94492b7c1586d32625e
> src/hdfs/hdfs.cpp 9c91d1f8c4f7dc54cd983245ab02a85466cb7e56
>
> Diff: https://reviews.apache.org/r/42246/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 42246: Fixed support for non-HDFS URIs by Hadoop
client.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42246/
-----------------------------------------------------------
(Updated Jan. 18, 2016, 3:20 a.m.)
Review request for mesos, haosdent huang, Jie Yu, and switched to 'mcypark'.
Changes
-------
Fixed all review issues.
Bugs: MESOS-4304
https://issues.apache.org/jira/browse/MESOS-4304
Repository: mesos
Description (updated)
-------
Review: https://reviews.apache.org/r/42246/
Diffs (updated)
-----
src/hdfs/hdfs.hpp abdb9b9013abc5e7c2e0d94492b7c1586d32625e
src/hdfs/hdfs.cpp 9c91d1f8c4f7dc54cd983245ab02a85466cb7e56
Diff: https://reviews.apache.org/r/42246/diff/
Testing
-------
make check
Thanks,
Bernd Mathiske
Re: Review Request 42246: Fixed support for non-HDFS URIs by Hadoop
client.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42246/#review114736
-----------------------------------------------------------
Ship it!
src/hdfs/hdfs.cpp (line 313)
<https://reviews.apache.org/r/42246/#comment175591>
+1 on update the comments.
I would suggest we also rename this method to 'normalize'.
Plus, this method does not need to be a member function. Can you make it a file local helper? I.e.,
```
// Normalize an HDFS path such that it is either an absolute
// path or a full hdfs:// URL.
static string normalize(const string& hdfsPath)
{
...
}
```
src/hdfs/hdfs.cpp (line 315)
<https://reviews.apache.org/r/42246/#comment175594>
This is a fine workaround at the current moment. Please add a NOTE here saying that the URLs for all protocols suppported by HDFS client contains "://".
- Jie Yu
On Jan. 13, 2016, 10:07 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42246/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 10:07 a.m.)
>
>
> Review request for mesos, haosdent huang, Jie Yu, and switched to 'mcypark'.
>
>
> Bugs: MESOS-4304
> https://issues.apache.org/jira/browse/MESOS-4304
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed support for non-HDFS URIs by Hadoop client.
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.cpp 51f016b049c6e947c4d27fbbbf79c53f9ec5a51e
>
> Diff: https://reviews.apache.org/r/42246/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 42246: Fixed support for non-HDFS URIs by Hadoop
client.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42246/#review114189
-----------------------------------------------------------
Patch looks great!
Reviews applied: [42246]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 13, 2016, 10:07 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42246/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 10:07 a.m.)
>
>
> Review request for mesos, haosdent huang, Jie Yu, and switched to 'mcypark'.
>
>
> Bugs: MESOS-4304
> https://issues.apache.org/jira/browse/MESOS-4304
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed support for non-HDFS URIs by Hadoop client.
>
>
> Diffs
> -----
>
> src/hdfs/hdfs.cpp 51f016b049c6e947c4d27fbbbf79c53f9ec5a51e
>
> Diff: https://reviews.apache.org/r/42246/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>