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

Reply via email to