[
https://issues.apache.org/jira/browse/HADOOP-10389?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14034560#comment-14034560
]
Haohui Mai edited comment on HADOOP-10389 at 6/17/14 11:05 PM:
---------------------------------------------------------------
bq. didn't integrate the existing JNIFS code, are not parsing the config XMLs,
don't handle errors, etc. etc.
Correct me if I'm wrong -- based on my understanding the original patch in
HADOOP-10369 implements none of them above. Both implementation can benefit
from other jiras (e.g. HADOOP-10446).
bq. ..., because you didn't implement generation of type-safe RPC wrappers (you
just wrote out everything manually in HdfsImpl::Exists),
This is implemented as C++ code in the original patch. It can be brought in if
it is required.
For what is worth, do you agree that the code implements comparable
functionality with much smaller code base?
bq. boost::asio doesn't provide equivalent functionality to libuv. It does wrap
a lot of network stuff, but doesn't handle things like condition variables,
signals, etc. etc., ..., For example, the boost condition variable actually can
let your thread wake up without recovering the lock (it then has to sleep
again).
I'm unsure why this is relevant as I don't see the current code uses condition
variables and signals. My guess is that you might need them to implement RPC
with timeout. I'm yet to be convinced that this is required in the c++
implementation, as {{std::future}} provides a much better abstraction for that.
bq. It uses exceptions, which are problematic.
This is not a fundamental part of the API. The current implementation uses the
exception-free variants of the library calls whenever possible. The
{{std::system_error}} object carries the error conditions.
The current implementation of the rpc-send path, however, might throw
{{bad_alloc}}. It can be fixed by limiting the maximum size of the outgoing
buffer.
I say it is safe to define {{BOOOST_NO_EXCEPTIONS}} and compile the code with
{{-fno-exceptions}}.
bq. Yes, you can use boost for some of that stuff, but the boost versions are
often not that good.
Boost in general has a versioning problem which is really bad for libraries. If
your library links against version X of boost, any application linking to you
needs to use the same version... otherwise bad stuff happens. You could link it
statically, but it might be too big for that to work (I've never tried).
The only thing required is {{boost::system::error}}, which have a fairly small
foolprints (~70K on Mac OS X). The size is comparable to the size of
{{protobuf-c}} (~75K on Mac OS X).
bq. To me this is not that interesting. There are a lot of clients already out
there in various languages... C, C++, even golang.
In my opinion the same argument applies to any clients, none of them is
superior to the others. Everybody can write their own clients. Contributions
are welcome, but for a community takes over the code and to maintain it, it is
important to ensure that the code can be easily changed, reviewed, and
maintained by other contributors.
This key point, however, remains unaddressed in the the current branch.
bq. If you're interested in something even smaller and more minimal, you can
check out this: https://github.com/toddlipcon/libhrpc/ Though I think that that
is for an out-of-date version of hadoop RPC at this point.
Given the fact that many hadoop RPC clients happily sit on github, and the
protocol remains pretty stable, is it more appropriate to host this project on
github if you don't have time to address the comments?
was (Author: wheat9):
bq. didn't integrate the existing JNIFS code, are not parsing the config XMLs,
don't handle errors, etc. etc.
Correct me if I'm wrong -- based on my understanding the original patch in
HADOOP-10369 implements none of them above. Both implementation can benefit
from other jiras (e.g. HADOOP-10446).
bq. ..., because you didn't implement generation of type-safe RPC wrappers (you
just wrote out everything manually in HdfsImpl::Exists),
This is implemented as C++ code in the original patch. It can be brought in if
it is required.
For what is worth, do you agree that the code implements comparable
functionality with much smaller code base?
bq. boost::asio doesn't provide equivalent functionality to libuv. It does wrap
a lot of network stuff, but doesn't handle things like condition variables,
signals, etc. etc., ..., For example, the boost condition variable actually can
let your thread wake up without recovering the lock (it then has to sleep
again).
I'm unsure why this is relevant as I don't see the current code uses condition
variables and signals. My guess is that you might need them to implement RPC
with timeout. I'm yet to be convinced that this is required in the c++
implementation, as {{std::future}} provides a much better abstraction for that.
bq. It uses exceptions, which are problematic.
This is not a fundamental part of the API. The current implementation uses the
exception-free variants of the library calls whenever possible. The
{{std::system_error}} object carries the error conditions.
The current implementation of the rpc-send path, however, might throw
{{bad_alloc}}. It can be fixed by limiting the maximum size of the outgoing
buffer.
I say it is safe to define {{BOOOST_NO_EXCEPTIONS}} and compile the code with
{{-fno-exceptions}}.
Yes, you can use boost for some of that stuff, but the boost versions are often
not that good.
Boost in general has a versioning problem which is really bad for libraries. If
your library links against version X of boost, any application linking to you
needs to use the same version... otherwise bad stuff happens. You could link it
statically, but it might be too big for that to work (I've never tried).
bq. The only thing required is {{boost::system::error}}, which have a fairly
small foolprints (~70K on Mac OS X). The size is comparable to the size of
{{protobuf-c}} (~75K on Mac OS X).
bq. To me this is not that interesting. There are a lot of clients already out
there in various languages... C, C++, even golang.
In my opinion the same argument applies to any clients, none of them is
superior to the others. Everybody can write their own clients. Contributions
are welcome, but for a community takes over the code and to maintain it, it is
important to ensure that the code can be easily changed, reviewed, and
maintained by other contributors.
This key point, however, remains unaddressed in the the current branch.
bq. If you're interested in something even smaller and more minimal, you can
check out this: https://github.com/toddlipcon/libhrpc/ Though I think that that
is for an out-of-date version of hadoop RPC at this point.
Given the fact that many hadoop RPC clients happily sit on github, and the
protocol remains pretty stable, is it more appropriate to host this project on
github if you don't have time to address the comments?
> Native RPCv9 client
> -------------------
>
> Key: HADOOP-10389
> URL: https://issues.apache.org/jira/browse/HADOOP-10389
> Project: Hadoop Common
> Issue Type: Sub-task
> Affects Versions: HADOOP-10388
> Reporter: Binglin Chang
> Assignee: Colin Patrick McCabe
> Attachments: HADOOP-10388.001.patch,
> HADOOP-10389-alternative.000.patch, HADOOP-10389.002.patch,
> HADOOP-10389.004.patch, HADOOP-10389.005.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.2#6252)
