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.

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

okhttp supports custom protocols. gh-809 #820

Closed
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.

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?

@OlgaMaciaszek OlgaMaciaszek added enhancement New feature or request and removed in progress labels Feb 1, 2023
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.

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.

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.

@@ -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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Are there any plans to support http/2.0
3 participants