Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Commons Developers List" <dev@commons.apache.org>
Received-SPF: error (athena.apache.org: local policy)
MIME-Version: 1.0
Sender: kristian.rosenvold@zenior.no
In-Reply-To: <54C22802.7000101@apache.org>
References: <87vbjy93rj.fsf@v35516.1blu.de>
	<54C22802.7000101@apache.org>
Date: Fri, 23 Jan 2015 19:33:08 +0100
Message-ID: 
 <CAJZRQKyvkmSi9U+Ui48=RWstk5m7YyVEFV5nY4vmY4pGZr2Krg@mail.gmail.com>
Subject: Re: [compress] Preparations for 1.10
From: Kristian Rosenvold <kristian.rosenvold@gmail.com>
To: Commons Developers List <dev@commons.apache.org>
Content-Type: text/plain; charset=UTF-8

2015-01-23 11:52 GMT+01:00 Emmanuel Bourg <ebourg@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-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org