New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

=htc simplify decompressor/deflate infrastructure #1570

Merged
merged 2 commits into from Nov 29, 2017

Conversation

Projects
None yet
4 participants
@jrudolph
Member

jrudolph commented Nov 28, 2017

The idea is to replace the handlers in DecompressorParsingLogic with simple parameters of the Inflate step.

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Nov 28, 2017

Member

These changes should be backported to the copy in akka-stream.

Member

jrudolph commented Nov 28, 2017

These changes should be backported to the copy in akka-stream.

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Nov 28, 2017

Member

/cc @tom-walford, I cleaned up a few things here also wrt the probing.

Member

jrudolph commented Nov 28, 2017

/cc @tom-walford, I cleaned up a few things here also wrt the probing.

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Nov 28, 2017

Collaborator

Test FAILed.

Collaborator

akka-ci commented Nov 28, 2017

Test FAILed.

@akka-ci akka-ci added validating and removed needs-attention labels Nov 28, 2017

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Nov 28, 2017

Member

The coding implementation classes were still public (accidentally, I'd say). So, I made them private to allow the binary incompatible changes. Strictly taken this is a violation of our binary compatibility guarantees. I'd say that it is unlikely that other code relies on that code but we cannot know of course. We could also postpone the actual changes to 10.1.0 but maybe that's overkill for these bits of infrastructure.

Member

jrudolph commented Nov 28, 2017

The coding implementation classes were still public (accidentally, I'd say). So, I made them private to allow the binary incompatible changes. Strictly taken this is a violation of our binary compatibility guarantees. I'd say that it is unlikely that other code relies on that code but we cannot know of course. We could also postpone the actual changes to 10.1.0 but maybe that's overkill for these bits of infrastructure.

@akka-ci akka-ci added needs-attention and removed validating labels Nov 28, 2017

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Nov 28, 2017

Collaborator

Test FAILed.

Collaborator

akka-ci commented Nov 28, 2017

Test FAILed.

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Nov 28, 2017

Member

PLS BUILD

Member

jrudolph commented Nov 28, 2017

PLS BUILD

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Nov 28, 2017

Collaborator

Test PASSed.

Collaborator

akka-ci commented Nov 28, 2017

Test PASSed.

@johanandren

LGTM with one question

override def afterInflate = inflateState
override def afterBytesRead(buffer: Array[Byte], offset: Int, length: Int): Unit = {}
private def examineAndBuildInflater(bytes: ByteString): Inflater = {
val wrapped = (bytes.head & 0x0F) == 0x08

This comment has been minimized.

@johanandren

johanandren Nov 29, 2017

Member

The old one took into account that the ByteString could be empty, but now we are sure it isn't?

@johanandren

johanandren Nov 29, 2017

Member

The old one took into account that the ByteString could be empty, but now we are sure it isn't?

This comment has been minimized.

@jrudolph

jrudolph Nov 29, 2017

Member

Yes, exactly. It's an invariant of ParseStep.parse that the byte reader will contain at least a single byte to read.

@jrudolph

jrudolph Nov 29, 2017

Member

Yes, exactly. It's an invariant of ParseStep.parse that the byte reader will contain at least a single byte to read.

jrudolph added some commits Nov 28, 2017

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Nov 29, 2017

Member

Added a section to upcoming release notes about the new incompatibility.

Member

jrudolph commented Nov 29, 2017

Added a section to upcoming release notes about the new incompatibility.

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Nov 29, 2017

Collaborator

Test FAILed.

Collaborator

akka-ci commented Nov 29, 2017

Test FAILed.

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Nov 29, 2017

Member

PLS BUILD

Member

jrudolph commented Nov 29, 2017

PLS BUILD

@akka-ci akka-ci added validating and removed needs-attention labels Nov 29, 2017

@akka-ci akka-ci added tested and removed validating labels Nov 29, 2017

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Nov 29, 2017

Collaborator

Test PASSed.

Collaborator

akka-ci commented Nov 29, 2017

Test PASSed.

@jrudolph jrudolph merged commit f575827 into akka:master Nov 29, 2017

3 checks passed

Jenkins PR Validation Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@jrudolph jrudolph deleted the jrudolph:jr/w/cleanup-deflate-infrastructure branch Nov 29, 2017

@tom-walford

This comment has been minimized.

Show comment
Hide comment
@tom-walford

tom-walford Dec 5, 2017

Contributor

Thanks for getting this all sorted @jrudolph :)

Contributor

tom-walford commented Dec 5, 2017

Thanks for getting this all sorted @jrudolph :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment