4206909 - adding Z_SYNC_FLUSH support to deflaters

[Martin Buchholz]

On Fri, Sep 11, 2009 at 16:13, Xueming Shen<Xueming.Shen at sun.com> wrote:
>
> I'm definitely open for any better choice, I'm not in hurry to close this
> one up. Sure, I hope we can
> reach a consensus before I go on vacation this weekend:-)
>
> So here is the planB alan is proposing, no API doc added yet, but you should
> know what it means. I will add the doc
> in later if we agree on this approach.
>
> http://cr.openjdk.java.net/~sherman/zipflush/webrev.planB/src/share/classes/java/util/zip/DeflaterOutputStream.java.sdiff.html

I vote for this approach - i.e. plan B as proposed by Alan.

I no longer advocate making partial flush the default,
as a result of my own arguments for the other side.

The use case for full flush is partial recoverability from future corruption
of the data.  If the competing java zib libraries are supporting this
flush variant,
we probably should too, for completeness.

David wrote:
> This is not unusual in the JDK or elsewhere - that a stream calls the underlying stream's flush() method I mean - and I think in the (unlikely) event that someone wants to flush the underlying stream only, they could simply call .flush() directly on that stream.

David: In this case the deflater is not an "underlying stream", and
the proposal was to
have flush() change the nature of the bytes written, while normally
OutputStream.flush merely causes
the same bytes to be written more promptly.

Martin "Every change is an incompatible change"

>
> We probably should figure out want to do with the GZipOutputStream...it
> probably needs something as well...we can figure
> out it later.
>
> So, Martin, Brandon, David and anyone interested,
>
> Is this something you guys can accept ? It makes the life easy for developer
> that needs the sync_flush for the flush(),
> you don't need to override/subclass anything, just makes it clear what you
> want when creating the DOS by picking
> the appropriate constructor, nothing more. Since we keep the default
> behavior un-changed, it is also compatible:-)
>
> It does not add the the full_sync, sure we can consider to use a enum
> instead of the boolean parameter, but since
> no one is asking for it now and the enum looks a little "too modern"
> compared to the rest of the API, we might
> want to stay with the simple "boolean doSyncFlush".
>
> Opinion?
>
> sherman
>
> Funny, I was "beaten up" today for one of my other changes, I was literally
> asked "where was it documented/highlighted
> that this fix was going to introduce an incompatibility", for a much much
> "less" incompatible change:-)
>
>
> Alan Bateman wrote:
>>
>> Xueming Shen wrote:
>>>
>>> I think we have enough discussion on this topic, it's time to make a
>>> final decision. Seems like we are all happy on the
>>> changes in Deflater and new DOS.flush(mode), the only difference is
>>> whether or not we should change the existing
>>> behavior of DOS.flush() should use Z_SYNC_FLUSH by default.
>>>
>>> Brandon is very firm on the "yes" side, strongly believe this is a MUST.
>>> Martin is on the fence, leaning towards "yes"
>>> Sherman is on the "no" side
>>>
>>> So if there is no other vote coming with strong opinion, your vote will
>>> be important:-) Are you also leaning to "yes"
>>> or "strongly" suggesting a yes? I need  strong "yes" to change my mind.
>>
>> This is tough call. It is clearly desirable to change flush() to work the
>> way that developers expect. On the other hand there is always risk with
>> changing long-standing behavior. The risk here is probably low but it's
>> impossible to quantify. Martin makes a good observation that it may cause
>> problems for code that wraps the stream with an auto-flushing PrintStream or
>> PrintWriter. You also made a good observation that just changing the number
>> of bytes written could cause breakage (in some fragile environment) - we
>> just don't know! So on balance, I think you are probably right to take the
>> conservative line on this one.
>>
>> I know you've had enough discussion on this but I wonder if you might be
>> open to considering new constructors for DeflaterOutputStream (and maybe
>> GZIPOutputStream) as an alternative to the flush(int) method. This might fit
>> better with this API, in particular for cases where the intended usage is
>> known. It could be as simple as a "boolean syncFlush" parameter to indicate
>> how uncompressed buffered bytes are handled when flushing (default to false
>> for existing constructors so existing behavior preserved).  The boolean
>> parameter is just an opening bid - clearly there are alternatives. The point
>> is that it makes it easy and more obvious as how to get a
>> DeflaterOutputStream suitable for interactive cases.
>>
>> -Alan.
>
>