Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

Configurable GzipCompressingInputStream compression buffer size #5848

Merged
merged 4 commits into from
Jan 11, 2022

Conversation

schlosna
Copy link
Contributor

Goals (and why):
GzipCompressingInputStream did not allow configuring compression buffer size, leading to suboptimal compression with default DeflaterInputStream buffer size of 512 bytes.

Implementation Description (bullets):
Allow configuring the GZIP compression buffer size. Increase default compression buffer size to 64 KiB.

Testing (What was existing testing like? What have you done to improve it?):
StreamCompressionTests

Concerns (what feedback would you like?): Is 64 KiB too large default? My sense is no, and that's the current default for LZ4 block size.

Where should we start reviewing?: GzipCompressingInputStream

Priority (whenever / two weeks / yesterday): this week

Allow configuring the GZIP compression buffer size. Increase default
compression buffer size to 64 KiB.
@changelog-app
Copy link

changelog-app bot commented Jan 11, 2022

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Configurable GzipCompressingInputStream compression buffer size

Goals (and why):
GzipCompressingInputStream did not allow configuring compression buffer size, leading to suboptimal compression with default DeflaterInputStream buffer size of 512 bytes.

Implementation Description (bullets):
Allow configuring the GZIP compression buffer size. Increase default compression buffer size to 64 KiB.

Testing (What was existing testing like? What have you done to improve it?):
StreamCompressionTests

Concerns (what feedback would you like?): Is 64 KiB too large default? My sense is no, and that's the current default for LZ4 block size.

Where should we start reviewing?: GzipCompressingInputStream

Priority (whenever / two weeks / yesterday): this week

Check the box to generate changelog(s)

  • Generate changelog entry

Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, though I think we should fix the changelog

for (int i = 0; i < data.length; i++) {
if (stream.read() != Byte.toUnsignedInt(data[i])) {
for (byte datum : data) {
if (stream.read() != Byte.toUnsignedInt(datum)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Comment on lines 3 to 25
description: |-
Configurable GzipCompressingInputStream compression buffer size

**Goals (and why)**:
`GzipCompressingInputStream` did not allow configuring compression buffer size, leading to suboptimal compression with default `DeflaterInputStream` buffer size of 512 bytes.

**Implementation Description (bullets)**:
Allow configuring the GZIP compression buffer size. Increase default compression buffer size to 64 KiB.

**Testing (What was existing testing like? What have you done to improve it?)**:
StreamCompressionTests

**Concerns (what feedback would you like?)**: Is 64 KiB too large default? My sense is no, and that's the current default for LZ4 block size.

**Where should we start reviewing?**: GzipCompressingInputStream

**Priority (whenever / two weeks / yesterday)**: this week

<!---
Please remember to:
- Add any necessary release notes (including breaking changes)
- Make sure the documentation is up to date for your change
--->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: |-
Configurable GzipCompressingInputStream compression buffer size
**Goals (and why)**:
`GzipCompressingInputStream` did not allow configuring compression buffer size, leading to suboptimal compression with default `DeflaterInputStream` buffer size of 512 bytes.
**Implementation Description (bullets)**:
Allow configuring the GZIP compression buffer size. Increase default compression buffer size to 64 KiB.
**Testing (What was existing testing like? What have you done to improve it?)**:
StreamCompressionTests
**Concerns (what feedback would you like?)**: Is 64 KiB too large default? My sense is no, and that's the current default for LZ4 block size.
**Where should we start reviewing?**: GzipCompressingInputStream
**Priority (whenever / two weeks / yesterday)**: this week
<!---
Please remember to:
- Add any necessary release notes (including breaking changes)
- Make sure the documentation is up to date for your change
--->
description: |-
Allow configuring the GZIP compression buffer size. Increase default compression buffer size to 64 KiB.

probably an oversight

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we need to update PR template, created #5849 to add the ==COMMIT_MSG== section

@jeremyk-91
Copy link
Contributor

🏎️ race condition

yeah this looks good 👍

@bulldozer-bot bulldozer-bot bot merged commit a1a92e9 into develop Jan 11, 2022
@bulldozer-bot bulldozer-bot bot deleted the ds/gzip-buffer-size branch January 11, 2022 17:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants