RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v3]

[Lance Andersen]

Hi Lin,

First thank you for your efforts on this feature.  As Alan suggested on March 26, we should take a step back and flush out the overall changes to the API before moving forward with additional PR reviews.   

We should look to see if it makes sense to use some of the more recent java features such as Record.

If we are adding a specific class to hold the header fields,  perhaps using the builder pattern should be considered vs  constructors.

If we are writing out these additional fields, what should we do with them when reading  a gzip file back?

Have you looked at other gzip api implementations to see what they provide in this area?    

Is there anyone who relies on this additional header info?  As I mentioned in an earlier comment, we should validate the changes against another implementation to ensure that we can read the data back correctly and that the additional header data that we write can be read by other tools.

Once we reach a consensus on the overall API changes and/or additions, then we can look to move forward with the next PR review.

Alan,  Yes I am happy to help Lin on moving this forward.

> On Apr 8, 2021, at 4:32 AM, Lin Zang <lzang at openjdk.java.net> wrote:
> 
> On Fri, 2 Apr 2021 08:53:20 GMT, Lin Zang <lzang at openjdk.org <mailto:lzang at openjdk.org>> wrote:
> 
>>> Hi Lin,
>>> 
>>> On Mar 24, 2021, at 2:51 AM, Lin Zang ***@***.******@***.***>> wrote:
>>> 
>>> 
>>> 
>>> Hi Lance，
>>> Thanks a lot for your review. I will update the PR ASAP.
>>> May I ask your help to also review the CSR？
>>> 
>>> I believe we need to flush out some of the issues I raised that were not test related as they will result in updates to the javadoc which will require an update to the CSR.
>>> 
>>> 
>>> 
>>> Thanks!
>>> 
>>> BRs,
>>> Lin
>>> 
>>> —
>>> You are receiving this because you commented.
>>> Reply to this email directly, view it on GitHub<https://github.com/openjdk/jdk/pull/3072#issuecomment-805549600>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABQPFO5R4INTAUFZ3QS6WL3TFGDXHANCNFSM4ZMLU6SA>.
>>> 
>>> ***@***.***
>>> 
>>> 
>>> 
>>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
>>> Oracle Java Engineering
>>> 1 Network Drive
>>> Burlington, MA 01803
>>> ***@***.******@***.***>
>> 
>> Dear @LanceAndersen,
>> Here I added a class `GZIPHeaderData` which is defined to hold all gzip header members, and make it an argument of the GZIPOutputStream constructor.  Would you like to help review and check whether this change looks reasonable?
>> 
>> BTW, this pr still lack of the negative test cases, I plan to add it when the constructor API is fixed.
>> 
>> Thanks!
>> Lin
> 
> Dear All,
> May I ask your help to review this change? Thanks!
> 
> BRs,
> Lin
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/3072 <https://git.openjdk.java.net/jdk/pull/3072>