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.
