Mailing-List: contact common-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: common-issues@hadoop.apache.org
Date: Fri, 23 Mar 2012 23:17:28 +0000 (UTC)
From: "Aaron T. Myers (Commented) (JIRA)" <jira@apache.org>
To: common-issues@hadoop.apache.org
Message-ID: 
 <98926271.11288.1332544648343.JavaMail.tomcat@hel.zones.apache.org>
In-Reply-To: 
 <1833032891.8844.1332522447757.JavaMail.tomcat@hel.zones.apache.org>
Subject: [jira] [Commented] (HADOOP-8202) stopproxy() is not closing the
 proxies correctly
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ 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