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

Migrate client transports to Apache HttpClient / Core 5.x #246

Merged
merged 1 commit into from
Nov 2, 2022
Merged

Migrate client transports to Apache HttpClient / Core 5.x #246

merged 1 commit into from
Nov 2, 2022

Conversation

reta
Copy link
Collaborator

@reta reta commented Oct 17, 2022 •
edited
Loading

Signed-off-by: Andriy Redko andriy.redko@aiven.io

Description

Migrate client transports to Apache HttpClient / Core 5.x

Issues Resolved

Closes #245

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has comments added
    • New functionality is added to docs folder
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@reta reta force-pushed the issues-245 branch 7 times, most recently from b40a3bd to 4631ea8 Compare October 24, 2022 16:15
@reta reta force-pushed the issues-245 branch 2 times, most recently from de4a8d5 to 32fd130 Compare October 25, 2022 20:15
reta
reta commented Oct 25, 2022
View reviewed changes
java-client/src/test/java/org/opensearch/client/opensearch/model/EndpointTest.java
@@ -51,10 +51,10 @@ public void testArrayPathParameter() {
assertEquals("/a/_refresh", RefreshRequest._ENDPOINT.requestUrl(req));

req = RefreshRequest.of(b -> b.index("a", "b"));
assertEquals("/a,b/_refresh", RefreshRequest._ENDPOINT.requestUrl(req));
assertEquals("/a%2Cb/_refresh", RefreshRequest._ENDPOINT.requestUrl(req));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The encoding path segments implementation has changed in Apache HttpClient 5

@reta
Migrate client transports to Apache HttpClient / Core 5.x
80180ed 
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
@reta reta marked this pull request as ready for review October 27, 2022 15:26
@reta
Copy link
Collaborator Author

reta commented Oct 27, 2022

@VachaShah when do you plan to branch off 2.x? thank you

@dblock
Copy link
Member

dblock commented Nov 1, 2022 •
edited
Loading

@reta So this is a breaking change for 3.0? If so, we can create a 2.x now - you should be able to do it. If not there's no need for a major version increment AFAIK.

@reta
Copy link
Collaborator Author

reta commented Nov 1, 2022

Thanks @dblock !

@reta So this is a breaking change for 3.0?

Yes

If so, we can create a 2.x now - you should be able to do it.

Yes, I can do it, just wanted to sync up with @VachaShah if that's planned already

@VachaShah
Copy link
Collaborator

Thanks @dblock !

@reta So this is a breaking change for 3.0?

Yes

If so, we can create a 2.x now - you should be able to do it.

Yes, I can do it, just wanted to sync up with @VachaShah if that's planned already

Hi @reta, you can go ahead and create a 2.x. Thank you!

@reta
Copy link
Collaborator Author

reta commented Nov 1, 2022

@VachaShah 2.x is branched off, would appreciate the review please when you have time, thank you!

@owaiskazi19
Copy link
Member

owaiskazi19 commented Nov 2, 2022 •
edited
Loading

@VachaShah @reta @dblock can we get this PR in? Build has been failing on SDK repo as it is using java-client

    java.lang.NoClassDefFoundError: org/apache/http/HttpHost
        at org.opensearch.sdk.SDKClient.initializeClient(SDKClient.java:38)
        at org.opensearch.sdk.TestSDKClient.testCreateClient(TestSDKClient.java:27)

        Caused by:
        java.lang.ClassNotFoundException: org.apache.http.HttpHost
            at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581)
            at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
            at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
            ... 2 more

cc: @joshpalis

@joshpalis joshpalis mentioned this pull request Nov 2, 2022
Closed
VachaShah
VachaShah approved these changes Nov 2, 2022
View reviewed changes
Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

Thank you @reta!

@VachaShah VachaShah merged commit c7b753e into opensearch-project:main Nov 2, 2022
joshpalis
joshpalis reviewed Nov 3, 2022
View reviewed changes
gradle.properties
systemProp.version = 3.0.0-SNAPSHOT
Copy link
Member

Choose a reason for hiding this comment

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

@reta We should remove the "-SNAPSHOT" from the system property and opensearch versions since this is already appended when published to sonatype.

Currently, 3.0.0 snapshots are being published incorrectly :

image

CC : @VachaShah

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@joshpalis check please #254

@owaiskazi19 owaiskazi19 mentioned this pull request Nov 3, 2022
Merged
@wbeckler
Copy link

wbeckler commented Nov 7, 2022

This looks like it could work as an option alongside the previous protocol in the opensearch-java 2.x line. As in, this could be added as a feature without forcing a major version upgrade. Thoughts?

@reta
Copy link
Collaborator Author

reta commented Nov 7, 2022 •
edited
Loading

This looks like it could work as an option alongside the previous protocol in the opensearch-java 2.x line. As in, this could be added as a feature without forcing a major version upgrade. Thoughts?

Not really, the opensearch-java uses RestClient from OpenSearch core, in 2.x it is still based on ApacheHttp Client 4.x. The support for both (4.x and 5.x) in core was discussed and concluded to rely on HttpClient 5.x only.

@owaiskazi19 owaiskazi19 mentioned this pull request Jan 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshpalis joshpalis joshpalis left review comments

@dblock dblock dblock approved these changes

@VachaShah VachaShah VachaShah approved these changes

@Bukhtawar Bukhtawar Awaiting requested review from Bukhtawar Bukhtawar is a code owner

@madhusudhankonda madhusudhankonda Awaiting requested review from madhusudhankonda madhusudhankonda is a code owner

@saratvemulapalli saratvemulapalli Awaiting requested review from saratvemulapalli saratvemulapalli is a code owner

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

Successfully merging this pull request may close these issues.

[FEATURE] Migrate client transports to Apache HttpClient / Core 5.x
6 participants
@reta @dblock @VachaShah @owaiskazi19 @wbeckler @joshpalis