hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aaron T. Myers (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-8202) stopproxy() is not closing the proxies correctly
Date Fri, 23 Mar 2012 23:17:28 GMT

    [ 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

        

Mime
View raw message