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

okhttp supports custom protocols. gh-809 #820

Closed
wants to merge 5 commits into from
Closed

okhttp supports custom protocols. gh-809 #820

wants to merge 5 commits into from

Conversation

galaxy-sea
Copy link
Contributor

hello @OlgaMaciaszek , Complete the OkHttpClient custom protocol, but Client.Default and ApacheHttp5Client do not support http2.

@OlgaMaciaszek OlgaMaciaszek self-assigned this Jan 31, 2023
@OlgaMaciaszek OlgaMaciaszek linked an issue Jan 31, 2023 that may be closed by this pull request
Are there any plans to support http/2.0 #809
Open
OlgaMaciaszek
OlgaMaciaszek requested changes Feb 1, 2023
View reviewed changes
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Hello, @galaxy-sea. Actually, HTTP2 is supported by OkHttp and it's enabled by default. However, this PR is still useful. I have added comments - please address them.

Also, there's support in Apache HC5. We should add the support for it within gh-809 (can be in a separate PR) - will you work on that as well?

...gn-core/src/test/java/org/springframework/cloud/openfeign/FeignOkHttpConfigurationTests.java Outdated Show resolved Hide resolved
...gn-core/src/test/java/org/springframework/cloud/openfeign/FeignOkHttpConfigurationTests.java Outdated Show resolved Hide resolved
...n-core/src/test/java/org/springframework/cloud/openfeign/protocol/FeignOkProtocolsTests.java Outdated Show resolved Hide resolved
...d-openfeign-core/src/test/java/org/springframework/cloud/openfeign/protocol/Application.java Outdated Show resolved Hide resolved
...n-core/src/test/java/org/springframework/cloud/openfeign/protocol/FeignOkProtocolsTests.java Outdated Show resolved Hide resolved
@OlgaMaciaszek OlgaMaciaszek added enhancement New feature or request and removed in progress labels Feb 1, 2023
OlgaMaciaszek
OlgaMaciaszek requested changes Feb 3, 2023
View reviewed changes
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks, @galaxy-sea. Have just added two cosmetic comments. Once this is done, we'll be able to merge it.

...n-core/src/test/java/org/springframework/cloud/openfeign/protocol/FeignOkProtocolsTests.java Outdated Show resolved Hide resolved
...n-core/src/test/java/org/springframework/cloud/openfeign/protocol/FeignOkProtocolsTests.java Outdated Show resolved Hide resolved
OlgaMaciaszek
OlgaMaciaszek reviewed Feb 3, 2023
View reviewed changes
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks, @galaxy-sea, I see the build is failing on checkstyle:
/home/runner/work/spring-cloud-openfeign/spring-cloud-openfeign/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignOkHttpConfigurationTests.java:20:1: 'javax.net.ssl.HostnameVerifier' should be separated from previous imports.

OlgaMaciaszek
OlgaMaciaszek requested changes Feb 3, 2023
View reviewed changes
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks, @galaxy-sea, the build is failing on checkstyle:
/home/runner/work/spring-cloud-openfeign/spring-cloud-openfeign/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignOkHttpConfigurationTests.java:20:1: 'javax.net.ssl.HostnameVerifier' should be separated from previous imports. . Have added a comment - please fix.

...gn-core/src/test/java/org/springframework/cloud/openfeign/FeignOkHttpConfigurationTests.java Outdated
@@ -17,10 +17,10 @@
package org.springframework.cloud.openfeign;

import java.lang.reflect.Field;

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is what is causing the build checkstyle failure - please revert this line deletion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I resubmitted the code

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem, thanks.

@OlgaMaciaszek
Copy link
Collaborator

@galaxy-sea I see there are some test failures that I would not expect as a result of your PR. Can you first merge changes from origin/main into your PR to get the updated artifact versions? If it still fails then, will take a closer look.

@galaxy-sea
Copy link
Contributor Author

@galaxy-sea I see there are some test failures that I would not expect as a result of your PR. Can you first merge changes from origin/main into your PR to get the updated artifact versions? If it still fails then, will take a closer look.

I submit

@OlgaMaciaszek
Copy link
Collaborator

Ok, @galaxy-sea - I'll take a look at it on Monday.

@OlgaMaciaszek OlgaMaciaszek mentioned this pull request Feb 7, 2023
Merged
@OlgaMaciaszek
Copy link
Collaborator

Hi, @galaxy-sea, this is happening because the Protocol class has been compiled without - parameters, which is necessary for AOT processing of the properties file. I have modified it to use Strings instead. Merged within gh-825. Thanks for your work on this issue @galaxy-sea.

@OlgaMaciaszek OlgaMaciaszek added closeable and removed enhancement New feature or request labels Feb 7, 2023
@galaxy-sea
Copy link
Contributor Author

hello @OlgaMaciaszek ，Thank you for solving my doubts. It seems that I need to learn AOT.

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

@OlgaMaciaszek OlgaMaciaszek OlgaMaciaszek requested changes

Assignees

@OlgaMaciaszek OlgaMaciaszek

Labels
closeable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Are there any plans to support http/2.0
3 participants
@galaxy-sea @OlgaMaciaszek @spring-cloud-issues