Unify exception/error handling in metrics-lib
There are now four different descriptor sources in metrics-lib: DescriptorParser
, DescriptorReader
, DescriptorCollector
, and DescriptorDownloader
.
We should think about unifying how we're handling exceptions and errors and telling the user about them. These thoughts should include whether or not to log exceptions and errors, though just doing that is probably not sufficient.
Let's go through the interfaces one by one:
public interface DescriptorParser {
public List<Descriptor> parseDescriptors(byte[] rawDescriptorBytes,
String fileName, boolean failUnrecognizedDescriptorLines)
throws DescriptorParseException;
}
The (slightly modified, as compared to current master) parseDescriptors
method handles a single file and throws an exception whenever something goes wrong. That works quite well, because that method blocks the caller, so they can easily wrap the call inside a try/catch
block.
That's somewhat different in the next interface:
public interface DescriptorCollector {
public void collectRemoteFiles(String collecTorBaseUrl,
String[] remoteDirectories, long minLastModified,
File localDirectory, boolean deleteExtraneousLocalFiles);
}
This method (which we should have called collectDescriptors
, in retrospect) blocks the caller, too, but it's unclear whether it should abort its operation when it hits the first exception. Right now it's rather silent about problems and only prints stack traces to System.err
. But that doesn't enable the caller to handle problems, neither. At least the method makes sure that it doesn't delete extraneous local files that might still exist remotely in case of an error.
The next interface is trickier:
public interface DescriptorReader {
public Iterator<Descriptor> readDescriptors(File[] directories,
File[] tarballs, SortedMap<String, Long> excludeFiles,
boolean failUnrecognizedDescriptorLines, int maxDescriptorsInQueue);
}
Note that this interface does not yet exist, but it's what I could imagine doing to simplify the current interface and make it more similar to the DescriptorCollector
interface. I could also imagine overloading this method.
The idea here is that the map passed in excludeFiles
would be updated while reading descriptors, so that the caller could use that to update a local history file. (This would be documented, of course.)
However, it's unclear how we would handle problems at all here. What we could do is extend the implementation of the returned Iterator<Descriptor>
to return a (runtime) exception whenever there was a problem reading the next descriptor that would be added next. Or maybe we should write our own Iterator
-like interface for returning descriptors and have its hasNextDescriptor()
and nextDescriptor()
methods throw DescriptorParseException
.
By the way, the current DescriptorReader
interface has a getExceptions()
method in the additional DescriptorFile
interface that is returned instead of Descriptor
. But that's something I'd like to get rid of, too. I also don't expect many users to look at those exceptions.
The last interface is the mostly dysfunctional DescriptorDownloader
interface. I could imagine that we handle exceptions/errors there similar to DescriptorReader
. Or we throw out that class, because it's only used by CollecTor, and generalizing its functionality might take more effort than writing it cleanly as part of CollecTor.
Thinking about the future, we might want to add another interface DescriptorWriter
, and we should think about ways to handle exceptions/errors there, too.
There, I mostly sketched out the problem here, without having good solutions. But maybe we can come up with good answers in this discussion?