4206909 - adding Z_SYNC_FLUSH support to deflaters

[Xueming Shen]

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

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.