I am new to Hadoop so please forgive me if I do not understand the philosophies behind this patch. If any of the close methods fail, they will throw an IOException which will be propagated up the stack. Isn't this the way all JAVA works?
This is indeed the way it works and is the desired behavior. The point of this patch is that when a close fails for any one of the Closeables, we should still make a last-ditch effort to close the others. If we can't close them then, there's nothing we can do.
1. In normal operation all close methods within the try will be called once, and then once again in the IOUtils.cleanup method. What purpose does this serve? I would rather the methods be called only once.
In the normal case all of the Closeables will be set to null. Note that IOUtils.cleanup(...) expressly handles nulls, and will not attempt to call close() again.
2. In the finally block, all IOExceptions which might have been thrown are logged, and then programmatically swallowed. The upstream functions are never made aware of these IOExceptions and I am not sure this is the right behavior.
It's true that in the exceptional case any failures to call close() in IOUtils.cleanup(...) will be logged and not propagated. This is exactly the intended behavior. Note that the original exception caused by the call to close() outside of IOUtils.cleanup(...) will still be propagated up.