Thanks for lots of great comments ! Just a few comments about some of the items;

Regarding the *Supplier interfaces, the basic idea is that once
commons-compress reaches jdk 1.8 language level (somewhere in the late
2030's :-) some of such interfaces may be modified to extend
java.util.function.Supplier. And if the supplier throws an exception
it'll be well defined on the interface with clear semantics. I somehow
think these interfaces serve to define the contract a little better
than just a straight callable throwing "Exception". It seems  like a
decent way to retromount some jdk8 idioms into jdk[567] compatible
libraries. I'm not really married to this solution, but I think it's
nicer than callable.

ZipArchiveEntryRequest is created for one specific purpose:
ZipArchiveEntry is created on a different thread that has no clear
happens-before relationship to the compressor thread, which means we
cannot really access any fields in that object unless we make them all
volatile. For a long time I figured I could get away without
ZipArchiveEntryRequest but I decided that the threading model would be
too hair-splitting and way too subtle for my liking; I prefer code
that does not require black-belt knowledge in memory model semantics.

I'll look at the specific code stuff for all the parallel code in the weekend.

Kristian


2015-01-23 11:52 GMT+01:00 Emmanuel Bourg <ebo...@apache.org>:
> 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
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Reply via email to