[
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
