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