Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Stream decoder optimize #1590

Merged
merged 35 commits into from
Apr 29, 2022
Merged

Stream decoder optimize #1590

merged 35 commits into from
Apr 29, 2022

Conversation

mroccyen
Copy link
Contributor

@mroccyen mroccyen commented Mar 7, 2022

This PR has the following optimizations:

  1. StreamDecoder has a strong validation for 'Stream' type that it is not conducive to scaling, so removal can be considered.
  2. Added a default implementation of the iterator encoder, providing the user with a choice, although users can also use their own iterator encoder.

@mroccyen mroccyen requested a review from velo March 8, 2022 06:29
velo
velo approved these changes Mar 17, 2022
View reviewed changes
Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

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

I will merge as is if you are tired of the back and forward, but if not =)

core/src/main/java/feign/stream/StreamDecoder.java Outdated Show resolved Hide resolved
@mroccyen mroccyen requested a review from velo March 21, 2022 03:03
kdavisk6
kdavisk6 requested changes Mar 24, 2022
View reviewed changes
Copy link
Member

@kdavisk6 kdavisk6 left a comment

Choose a reason for hiding this comment

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

@mroccyen I have some maintenance related feedback.

core/src/main/java/feign/stream/StreamDecoder.java Outdated Show resolved Hide resolved
core/src/main/java/feign/stream/StreamDecoder.java Outdated Show resolved Hide resolved
@mroccyen mroccyen requested a review from kdavisk6 March 25, 2022 11:31
@kdavisk6
Copy link
Member

Thank you for updating. If you could please add a section to the README with some documentation, this PR looks ready to be merged.

kdavisk6
kdavisk6 previously requested changes Mar 25, 2022
View reviewed changes
Copy link
Member

@kdavisk6 kdavisk6 left a comment

Choose a reason for hiding this comment

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

Please add some documentation to the Readme

velo
velo approved these changes Mar 25, 2022
View reviewed changes
core/src/main/java/feign/stream/StreamDecoder.java Outdated Show resolved Hide resolved
@mroccyen mroccyen requested a review from velo March 27, 2022 14:09
@mroccyen mroccyen requested a review from kdavisk6 March 27, 2022 14:39
@mroccyen
Copy link
Contributor Author

Please add some documentation to the Readme

I've already added it.

velo
velo reviewed Apr 29, 2022
View reviewed changes
core/src/main/java/feign/stream/StreamDecoder.java Outdated
this.iteratorDecoder = iteratorDecoder;
this.delegateDecoder = delegateDecoder != null
Copy link
Member

Choose a reason for hiding this comment

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

this.delegateDecoder = Optional.ofNullable(delegateDecoder);

@velo velo merged commit 7230f7c into OpenFeign:master Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@velo velo velo approved these changes

@kdavisk6 kdavisk6 Awaiting requested review from kdavisk6

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mroccyen @kdavisk6 @velo