commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Bodewig <bode...@apache.org>
Subject Re: [compress] Importance of retaining exact compatibility for ZipArchiveOutputStream ?
Date Mon, 29 Dec 2014 14:05:47 GMT
On 2014-12-29, Kristian Rosenvold wrote:

> The refactoring os ZipArchiveOutputStream to use StreamCompressor is now done in
> the branch https://github.com/krosenvold/commons-compress

Some code comments:

* the fields writtenToOutputStream, sourcePayloadLength and actualCrc in
  StreamCompressor can probably be made private
* inside the streams we usually write to the underlying stream first and
  update the count later - so the count is never bigger than what has
  been written if the write throws an exception.  I don't think this
  also applied to StreamCompressor's writeCounted but may be nice for
  consistency.
* ZipArchiveOutputStream's Deflater has been protected and is now
  removed.  We'll need to discuss whether we all can accept the
  potentially breaking change.

> As refactorings come it doesn't "feel" too good (extracting all the
> methods to create zip headers to a different class would probably be a
> refactoring with a lot better *feeling* to it....). This may be due to
> the overall largeness of the file in question, it may also be because
> of the "leaks" between ZipArchiveOutputStream (fields raf & out) and
> StreamCompressor.

You could probably remove the out field if you added a flush method to
the stream compressor.  But the raf field remains for rewriting sizes in
seekable output - I don't see how this could be moved to
StreamCompressor without feeling strange.

I'd like to see the OutputStream and RandomAccessFile versions of
StreamCompressor#create go away in favour of BackingStore
implementations, this could remove the inner subclasses at the same
time.  The RandomAccessFile case could probably be merged into
FileBasedBackingStore where the BackingStore preferred RandomAccessFile
when available and falls back to FileOutputStream.  Unfortunately that
would just duplicate the logic from ZipArchiveOutputStream since that
one still needed access to the RandomAccessFile.

Yes, I can see how splitting out the code that writes the metadata parts
of the archive from those that write actual file data might help.  The
later part could again be separated for DEFLATED and STORED.  What I
don't see is how to do it in a backwards compatible manner.

Stefan

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


Mime
View raw message