Le 22/01/2015 17:30, Stefan Bodewig a écrit : > Please have a look and identify stuff that looks as if I'd have to > reroll a new RC should it come to a vote with the current code base.
I reviewed the API changes, here are my comments: - PasswordRequiredException: the exception is in the sevenz package, do we want to move it elsewhere so it can be used later for other formats like zip (if that makes sense). - there are some missing @since tags on the new classes (ScatterGatherBackingStoreSupplier, ParallelScatterZipCreator, InputStreamSupplier, ZipArchiveEntryRequest, ZipArchiveEntryPredicate) - InputStreamSupplier could have been replaced with Supplier<InputStream> if we were using Java 8. But with the current JDK I wonder if we could use Callable<InputStream> instead to spare an interface. - If we had a kind of DeferredInputStream (not yet in commons-io sadly) the API could be slightly simpler. ZipArchiveEntryRequest and InputStreamSupplier could go away. DeferredInputStream would open an underlying stream only when a read() method is called. - ParallelScatterZipCreator has two public methods with a Callable<Object> in the signature, but it's not clear what this Object actually is. - Is ParallelScatterZipCreator.getStatisticsMessage() intended to remain in the final API or was it just there for benchmarking the implementation during the development? If it is meant to stay maybe a proper ScatterStatistics class would be cleaner. - ParallelScatterZipCreator.createCallable: the message of the IllegalArgumentException could mention the name of the entry involved. - The javadoc of the default ParallelScatterZipCreator constructor could explain how the thread pool is sized by default. - Could ScatterGatherBackingStoreSupplier be avoided? The public ParallelScatterZipCreator constructor that uses it could accept a ScatterGatherBackingStore instead, we would just have to ensure that the backing store doesn't open resources until it's used. - ScatterGatherBackingStore, FileBasedScatterGatherBackingStore and ScatterGatherBackingStoreSupplier are in the zip package but have nothing specific to the zip format. Should we move them elsewhere so they can be used later to implement parallel compression for other formats? - BitInputStream: why not using a long cache instead of an int, like BitStream before the refactoring? It might be interesting to do some benchmarks and see if it make a difference. - I think an example demonstrating the parallel zip creation would be a nice addition to the site. Emmanuel Bourg --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
