hadoop-common-issues mailing list archives

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

    [ https://issues.apache.org/jira/browse/HADOOP-8202?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13237207#comment-13237207
] 

Suresh Srinivas commented on HADOOP-8202:
-----------------------------------------

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.

bq. Can you guarantee that all future invocation handlers in Hadoop will implement Closeable?
Write the code with either proxy is closeable or has an invocation handler that is closeable.
If that is not the case, then it is programming error! Throw RuntimeException, so it is found
early and not silently ignored.

bq. This is a great reason to add a test - so that such things don't regress in the future.
HADOOP-7607 did not add tests either. Hence this bug. I suggest, lets practice what we preach!

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. Adding useless
tests comes at a cost. When we did the federation feature, we spent half the time fixing poorly
documented and lame tests. Not that these tests were finding bugs, the tests simply did not
work well the code changes.

That said, Hari, if you want you can add tests.

bq. Sure, it's a preference, but that in itself isn't a good reason to not address the comment.
Can you comment on why it's better to split the reasons for an error across several log statements?
You cannot argue matter of taste. Is the code incorrect? Else, I like this code better, because
I do not have to handle exception twice, but in one single place around closeable. 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.

bq. If you have a better way to implement RPC.getServerAddress, I'm happy to hear it.
Perhaps this is the only way. I need to look into it. But that is not required while stopping
proxy. It is orthogonal.

Hari, after thinking a bit, I believe we should throw HadoopIllegalArgumentException, if either
the proxy is not closeable or does not have Invocation handler.

                
> 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.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