2015-01-23 11:52 GMT+01:00 Emmanuel Bourg <ebo...@apache.org>: > - 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).
Makes sense, where should we put that ? > - there are some missing @since tags on the new classes > (ScatterGatherBackingStoreSupplier, ParallelScatterZipCreator, > InputStreamSupplier, ZipArchiveEntryRequest, ZipArchiveEntryPredicate) Fixed r1654291 > - 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. Discussed in separate mail; leaving as-is for now. > - 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. I think the overall memory model constraints in a parallel context favour the "Supplier" approach, since you avoid the entire need for a thread-safe DeferredInputStream. > - ParallelScatterZipCreator has two public methods with a > Callable<Object> in the signature, but it's not clear what this Object > actually is. Fixed in r1654291 > > - 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. Introduced class in r1654291 > > - ParallelScatterZipCreator.createCallable: the message of the > IllegalArgumentException could mention the name of the entry involved. Also fixed. > - The javadoc of the default ParallelScatterZipCreator constructor could > explain how the thread pool is sized by default. Also fixed. > - 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. Concurrent running creates multiple instances, so that won't work. > - 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? Good idea; something like org.apache.commons.compress.archivers.concurrent or org.apache.commons.compress.concurrent ? > - I think an example demonstrating the parallel zip creation would be a > nice addition to the site. I'll add some code later tonight. Thanks a lot for some great comments ! Kristian --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
