Juan Hernandez has posted comments on this change.
Change subject: core: Tar cleanup
......................................................................
Patch Set 3:
I think that using a method like "IOUtils.closeQuietly()" is better than what
we have now. However there is one thing I don't like about it: the "quietly"
part. I mean, if there is an exception while closing a stream I want to have it
clearly visible in the log, with some information that helps understand where
this is happening.
I know that it is extremely unlikely to have an error when closing something,
but if that happens I want to know.
In the past I have used a family of "close()" methods that take the stream (or
the closeable) and additional information, for example:
void closeNotQuitely(Closeable closeable, File file) {
// If no closeable is given just warn:
if (closeable == null) {
Throwable exception = new Throwable();
log.warn("Stream is null while closing file \"" +
file.getAbsolutePath() + "\".", exception);
return;
}
// Try to close the object and warn in the log if it is
// not possible:
try {
closeable.close();
}
catch (IOException exception) {
log.warn("Unexpected exception while trying to close file \"" +
file.getAbsolutePath() + "\".", exception);
}
}
A method like this leaves a trace of the unexpected problem in the log with
information of what file caused it. I am in favor of replacing all the places
where we use this "finally-close" logic with a call to one of this methods, but
only if it is "closeNotQuietly" or similar. I suggest to add these methods to
FileUtils (or to a new IOUtils class).
Regarding the inclusion of the commons-io dependency I am not against it, but
we already have a "close" method in "FileUtils". If we finally decide to
include commons-io remember that the .spec file of the RPMs needs to be updated
accordingly, it is not enough with updating the POM files.
Regarding the use of the try-with-resources idiom I have nothing against it, in
fact I like it, but it has been decided some time ago to keep the source code
Java 6 compatible, in case we need to rollback to Java 6 (note that this is
strictly required for UI code, as GWT doesn't support Java 7). You may want to
reopen this discussion in engine-devel for release 3.2.
--
To view, visit http://gerrit.ovirt.org/8211
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I766b94086e4696a035b99415f599b16ae78ab94f
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
