[
https://issues.apache.org/jira/browse/HADOOP-8202?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13237278#comment-13237278
]
Aaron T. Myers commented on HADOOP-8202:
----------------------------------------
bq. The problem with the code that was written is, it silently ignored the
error and just printed a log and did not indicate the error. This is what is
being fixed now.
Note that when HADOOP-7607 was being implemented, it was a conscious decision
to log a warning instead of throwing, so as to maintain backward compatibility
with the previous implementation. See [this
comment|https://issues.apache.org/jira/browse/HADOOP-7607?focusedCommentId=13097236&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13097236].
We're now making a conscious decision to change that here, which is fine, but
it should be noted.
bq. HADOOP-7607 did not add tests either. Hence this bug. I suggest, lets
practice what we preach!
HADOOP-7607 didn't introduce this bug. HADOOP-7607 was just a refactor, hence
why it had no tests.
This bug was introduced because of the following events:
# It used to be that all proxy objects used by client classes were directly
referenced by RPC engines. During this time, the code in RPC#stopProxy worked
just fine, but required users of proxy objects that wrapped other proxy objects
to hold a reference to the underlying proxy object just for the purpose of
calling RPC#stopProxy. This is why for a long time DFSClient had two references
to ClientProtocol objects - one to call methods on, the other to call
RPC#stopProxy on.
# HADOOP-7607 refactored the code in RPC#stopProxy to actually call close on
the invocation handler of the proxy object directly, instead of going through
the RPCEngine. This would allow the invocation handler to bubble this down to
underlying proxies. This was committed in September of 2011.
# We began to introduce protocol translators for protobuf support in December
of 2011. This caused the code in RPC#stopProxy to stop actually releasing
underlying resources when RPC#stopProxy was called with a translator object
provided as the argument, since the objects we were calling RPC#stopProxy on
were no longer actual proxy objects, and hence Proxy#getInvocationHandler would
fail. Unfortunately, no one noticed this until now. The introduction of
protocol translators also caused RPC#getServerAddress to break, but again no
one noticed. I introduced the ProtocolTranslator interface because I needed
RPC#getServerAddress to work in order to implement HDFS-2792.
bq. On a related note, I am also not happy for simple changes, we keep
mandating adding unit tests. Some times, it is okay to use judgement call and
not add unnecessary tests.
Sure, I agree with you. The question here is what constitutes a "simple change."
FWIW, we often add tests for "simple changes," not to prove that the new code
works, but to prevent it from ever regressing in the future. See HDFS-3099 as
an example.
bq. I am observing that our code reviews are becoming too strict. Not every
patch I review should look like the code I would write. As long it is correct,
follows coding standards, it should be good. I have been seeing some comments
these days, to say, can we call variable name as ioe instead of e. I believe,
we should relax these.
Sure, they're definitely just nits, but it's not like they're difficult to
address. It almost certainly would take less time to address these comments
than it does to argue about them.
> stopproxy() is not closing the proxies correctly
> ------------------------------------------------
>
> Key: HADOOP-8202
> URL: https://issues.apache.org/jira/browse/HADOOP-8202
> Project: Hadoop Common
> Issue Type: Bug
> Components: ipc
> Affects Versions: 0.24.0
> Reporter: Hari Mankude
> Assignee: Hari Mankude
> Priority: Minor
> Attachments: HADOOP-8202-1.patch, HADOOP-8202-2.patch,
> HADOOP-8202.patch, HADOOP-8202.patch
>
>
> I was running testbackupnode and noticed that NNprotocol proxy was not being
> closed. Talked with Suresh and he observed that most of the protocols do not
> implement ProtocolTranslator and hence the logic in stopproxy() does not
> work. Instead, since all of them are closeable, Suresh suggested that
> closeable property should be used at close.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
