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
