[FEATURE] Introduce new OpenSearchTransport based on Apache HttpClient 5 #281

reta · 2022-11-25T16:23:02Z

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

Description

Get rid of the dependency on OpenSearch core. Introducing new OpenSearchTransport based on Apache HttpClient 5, with no dependencies on OpenSearch's core RestClient.

Issues Resolved

PoC for #262

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 · 2022-11-25T16:25:33Z

@dblock @dlvenable the PoC for getting rid of the dependency on OpenSearch core, basically more or less the amount of code we would need to copy / paste from core (I think I could reduce it a bit but that would not be a game changer there). It should help us to assess the go / no-go decision.

dblock

I love it. It's a lot smaller than I thought it would be and it's mostly boilerplate (+ tests?).

reta · 2022-11-28T19:58:43Z

I love it. It's a lot smaller than I thought it would be and it's mostly boilerplate (+ tests?).

Sure, a few sketches needed to make it compete and testable (just wanted to hear your opinion first), I'll wrap it up shortly.

reta · 2022-12-01T20:09:25Z

@dblock @VachaShah got stuck on this one #47 :(

andrross · 2022-12-02T16:34:25Z

I love this as well. Removing the dependency on core clearly outweighs any downside to code duplication, in my opinion.

reta · 2023-01-11T19:43:32Z

@dblock @andrross @VachaShah I think the POC is ready

dblock

This is real progress!

What's the path to delete https://github.com/opensearch-project/opensearch-java/blob/f1fbe833d7cd925018b7ed3f9cd1c0684e25b993/java-client/build.gradle.kts#LL149C5-L149?

dblock · 2023-01-12T20:20:09Z

...ient/src/main/java/org/opensearch/client/transport/httpclient5/ApacheHttpClient5Options.java

+import org.opensearch.client.transport.Version;
+
+public class ApacheHttpClient5Options implements TransportOptions {
+    private static final String USER_AGENT = "User-Agent";


I found it interesting that the client does not declare things like user-agent as constants. Since this is only used once, maybe we should too? I noticed you don't for "Accept" :)

I have largely copied it from RestClientOptions, we probably should have constants, I agree

dblock · 2023-01-12T20:23:05Z

...ient/src/main/java/org/opensearch/client/transport/httpclient5/ApacheHttpClient5Options.java

+        String ua = String.format(
+            Locale.ROOT,
+            "opensearch-java/%s (Java/%s)",
+            Version.VERSION == null ? "Unknown" : Version.VERSION.toString(),


Nit: should never be null

@dblock you will be surprised but [1]:

/** * This library's version, read from the classpath. Can be {@code null} if the version resource could not be read. */ @Nullable public static final Version VERSION;

[1] https://github.com/opensearch-project/opensearch-java/blob/main/java-client/src/main/java/org/opensearch/client/transport/Version.java#L138-L139

reta · 2023-01-12T20:56:51Z

This is real progress!

What's the path to delete https://github.com/opensearch-project/opensearch-java/blob/f1fbe833d7cd925018b7ed3f9cd1c0684e25b993/java-client/build.gradle.kts#LL149C5-L149?

Thanks @dblock ! This is what we have discussed here [1], basically easy steps:

for 3.0.0, make RestClient optional (as the dependency)
recommend to use Apache HttpClient 5 based transport instead

[1] opensearch-project/OpenSearch#5424 (comment)

dblock

I'm good to merge this or wait to get the actual cut-off done on main.

java-client/src/main/java/org/opensearch/client/transport/httpclient5/DeadHostState.java

andrross · 2023-01-12T22:51:50Z

This is real progress!
What's the path to delete https://github.com/opensearch-project/opensearch-java/blob/f1fbe833d7cd925018b7ed3f9cd1c0684e25b993/java-client/build.gradle.kts#LL149C5-L149?

Thanks @dblock ! This is what we have discussed here [1], basically easy steps:
* for 3.0.0, make RestClient optional (as the dependency)

* recommend to use Apache HttpClient 5 based transport instead
[1] opensearch-project/OpenSearch#5424 (comment)

I may be confused, but that's the plan for removing the legacy rest client from OpenSearch right? Why can't we remove the dependency on OpenSearch from opensearch-java right now?

reta · 2023-01-13T00:47:15Z

This is real progress!
What's the path to delete https://github.com/opensearch-project/opensearch-java/blob/f1fbe833d7cd925018b7ed3f9cd1c0684e25b993/java-client/build.gradle.kts#LL149C5-L149?

Thanks @dblock ! This is what we have discussed here [1], basically easy steps:
* for 3.0.0, make RestClient optional (as the dependency)

* recommend to use Apache HttpClient 5 based transport instead
[1] opensearch-project/OpenSearch#5424 (comment)
I may be confused, but that's the plan for removing the legacy rest client from OpenSearch right? Why can't we remove the dependency on OpenSearch from opensearch-java right now?

That would be hard breaking change: since we never deprecated RestClientTransport, just removing it without any notice is probably not a good idea. We at least could start with deprecating RestClientTransport, and removing in the next version, I think that would be less impactful.

dblock · 2023-01-13T17:49:10Z

Thanks @reta @andrross and @dlvenable! Let's talk about what we want to merge. I think we should

Make this PR as "added OpenSearchTransport", document how to use it, merge
In a separate PR, "deprecate RestClientTransport (and possibly other classes)", and mark the deprecated classes as such, document the recommended way to use the client ([FEATURE] Deprecate RestClientTransport #326)
Backport 1) and 2) to 2.x, continue a 2.x line of releases.
In 3.0/main delete the deprecated code.

reta · 2023-01-13T18:12:50Z

Is it possible to do this in a way where the public-facing part of the API doesn't have to change when the underlying Apache HttpClient changes? i.e. call the public API ApacheHttpClientTransport That way users don't have to change their code when the dependency is updated.

This is not really possible, the Apache HttpClient 4 and Apache HttpClient 5 are very different: we could hide some (like fe HttpHost) but everything from credentials to TLS configuration does require package change. However, if we do ApacheHttpClient4Transport in 2.x, we could keep it in 3.x as well - zero change for users.

andrross · 2023-01-13T18:29:33Z

Is it possible to do this in a way where the public-facing part of the API doesn't have to change when the underlying Apache HttpClient changes? i.e. call the public API ApacheHttpClientTransport That way users don't have to change their code when the dependency is updated.

This is not really possible, the Apache HttpClient 4 and Apache HttpClient 5 are very different: we could hide some (like fe HttpHost) but everything from credentials to TLS configuration does require package change. However, if we do ApacheHttpClient4Transport in 2.x, we could keep it in 3.x as well - zero change for users.

Makes sense. I'm slowly getting up to speed here :) So HttpClient 4 and 5 are actually distinct packages that can exists side-by-side (org.apache.httpcomponents.httpclient vs org.apache.httpcomponents.client5.httpclient5). Is that right? Assuming that's true, why is it a breaking change to add the new HttpClient 5 dependency in 2.x?

reta · 2023-01-13T18:38:07Z

 Makes sense. I'm slowly getting up to speed here :) So HttpClient 4 and 5 are actually distinct packages that can exists side-by-side (org.apache.httpcomponents.httpclient vs org.apache.httpcomponents.client5.httpclient5). Is that right?

That's right.

Assuming that's true, why is it a breaking change to add the new HttpClient 5 dependency in 2.x?

This is true. There are no concerns regarding breaking change, my concern is "new dependencies are going to be introduced", it may be very surprising to see that within minor version update (may be I am overly cautious).

andrross · 2023-01-13T18:58:52Z

Assuming that's true, why is it a breaking change to add the new HttpClient 5 dependency in 2.x?

This is true. There are no concerns regarding breaking change, my concern is "new dependencies are going to be introduced", it may be very surprising to see that within minor version update (may be I am overly cautious).

The tl;dr is that this PR will add a new dependency (Apache HttpClient 5) and create a new ApacheHttpClient5Transport class that will be the new recommended approach for creating a client, as opposed to the existing RestClientTransport that depends on OpenSearch core. The specific question is whether adding the HttpClient5 dependency should be considered a breaking change, or can it be added in the next minor release on 2.x? @wbeckler @dlvenable @dblock What do you think?

reta · 2023-01-13T19:02:23Z

Assuming that's true, why is it a breaking change to add the new HttpClient 5 dependency in 2.x?

This is true. There are no concerns regarding breaking change, my concern is "new dependencies are going to be introduced", it may be very surprising to see that within minor version update (may be I am overly cautious).

The tl;dr is that this PR will add a new dependency (Apache HttpClient 5) and create a new ApacheHttpClient5Transport class that will be the new recommended approach for creating a client, as opposed to the existing RestClientTransport that depends on OpenSearch core. The specific question is whether adding the HttpClient5 dependency should be considered a breaking change, or can it be added in the next minor release on 2.x? @wbeckler @dlvenable @dblock What do you think?

Please note that right now Apache HttpClient 5 comes with OpenSearch core 3.0.0, nothing is added to opensearch-java (as you can see, the build dependencies have not changed in this pull request).

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

andrross

Thanks @reta, this looks good to me!

I believe the remaining open question is whether to backport this to 2.x (with a slight change to explicitly declare the HttpClient5 dependencies) or instead introduce an ApacheHttpClient4Transport. In either case this change will be needed on main.

dlvenable · 2023-01-13T20:55:26Z

@reta , I did not actually see a dependency change in this PR (no modification to the build file). It seems that we are pulling in Apache HTTP Client 5 via a transitive dependency.

We may want to explicitly add this dependency now that the project itself depends on it.

Thanks @reta, this looks good to me!

I believe the remaining open question is whether to backport this to 2.x (with a slight change to explicitly declare the HttpClient5 dependencies) or instead introduce an ApacheHttpClient4Transport. In either case this change will be needed on main.

Is this dependency already in the 2.x line? If so, we are not adding a new dependency. If not, we can make this optional at runtime to address the concern of adding new dependencies. End users would need to add it to their build files to use the transport.

dlvenable · 2023-01-13T20:58:16Z

The specific question is whether adding the HttpClient5 dependency should be considered a breaking change, or can it be added in the next minor release on 2.x?

In my opinion, this is not breaking since it is compatible with HttpClient4. If we really want, it is possible to make this an optional dependency on 2.x using feature variants. End users would have to add this dependency or get a runtime error.

andrross · 2023-01-13T21:05:44Z

@dlvenable We are getting HttpClient5 on the main branch via a transitive dependency. For the 2.x branch it would have to be added a new dependency.

dblock · 2023-01-13T21:59:33Z

I merged it. @reta backport or not?

reta · 2023-01-13T23:49:37Z

I merged it. @reta backport or not?

Let me try to do this on Monday (the automatic one won't work, we need explicit dependencies), thanks @dblock !

…earch-project#281) Signed-off-by: Andriy Redko <andriy.redko@aiven.io> Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

…#328) Signed-off-by: Andriy Redko <andriy.redko@aiven.io> Signed-off-by: Andriy Redko <andriy.redko@aiven.io> Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

dblock reviewed Nov 28, 2022

View reviewed changes

reta mentioned this pull request Nov 30, 2022

[FEATURE] Get rid of the dependency on OpenSearch core #262

Open

reta force-pushed the issue-262 branch 5 times, most recently from 25b97c0 to 9364982 Compare December 1, 2022 20:08

reta force-pushed the issue-262 branch from 9364982 to 713ab88 Compare December 1, 2022 20:26

reta force-pushed the issue-262 branch 5 times, most recently from e0cf2c2 to 051b2b1 Compare January 11, 2023 19:16

reta changed the title ~~[WIP] [FEATURE] Get rid of the dependency on OpenSearch core~~ [FEATURE] Get rid of the dependency on OpenSearch core Jan 11, 2023

reta marked this pull request as ready for review January 11, 2023 19:34

reta requested review from Bukhtawar, madhusudhankonda, saratvemulapalli and VachaShah as code owners January 11, 2023 19:34

dblock reviewed Jan 12, 2023

View reviewed changes

dblock previously approved these changes Jan 12, 2023

View reviewed changes

andrross reviewed Jan 12, 2023

View reviewed changes

java-client/src/main/java/org/opensearch/client/transport/httpclient5/DeadHostState.java Outdated Show resolved Hide resolved

reta force-pushed the issue-262 branch 2 times, most recently from d593b64 to b557835 Compare January 13, 2023 18:19

reta changed the title ~~[FEATURE] Get rid of the dependency on OpenSearch core by introducing Apache HttpClient 5 based transport~~ [FEATURE] Introduce new OpenSearchTransport based on Apache HttpClient 5 Jan 13, 2023

reta force-pushed the issue-262 branch from b557835 to f80e663 Compare January 13, 2023 19:02

Introduce new OpenSearchTransport based on Apache HttpClient 5

e95c44a

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

reta force-pushed the issue-262 branch from f80e663 to e95c44a Compare January 13, 2023 19:04

andrross approved these changes Jan 13, 2023

View reviewed changes

dlvenable approved these changes Jan 13, 2023

View reviewed changes

dblock approved these changes Jan 13, 2023

View reviewed changes

dblock merged commit 9657df6 into opensearch-project:main Jan 13, 2023

reta mentioned this pull request Jan 16, 2023

[Backport] [2.x] Introduce new OpenSearchTransport based on Apache HttpClient 5 (#281) #328

Merged

reta mentioned this pull request Jan 16, 2023

Minor fixes related to ApacheHttpClient5Transport #329

Merged

reta mentioned this pull request Jan 16, 2023

[META] Support for HTTP/2 (client side) opensearch-project/OpenSearch#4257

Closed

ls-urs-keller mentioned this pull request Jan 19, 2024

Add wrapper to builder ls-urs-keller/opensearch-java#1

Closed

BrendonFaleiro mentioned this pull request Jun 21, 2024

2.x #1045

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Introduce new OpenSearchTransport based on Apache HttpClient 5 #281

[FEATURE] Introduce new OpenSearchTransport based on Apache HttpClient 5 #281

reta commented Nov 25, 2022 •

edited

Loading

reta commented Nov 25, 2022

dblock left a comment

reta commented Nov 28, 2022 •

edited

Loading

reta commented Dec 1, 2022

andrross commented Dec 2, 2022

reta commented Jan 11, 2023

dblock left a comment

dblock Jan 12, 2023

reta Jan 12, 2023

reta Jan 13, 2023

dblock Jan 12, 2023

reta Jan 12, 2023 •

edited

Loading

reta commented Jan 12, 2023

dblock left a comment

andrross commented Jan 12, 2023

reta commented Jan 13, 2023

dblock commented Jan 13, 2023 •

edited

Loading

reta commented Jan 13, 2023

andrross commented Jan 13, 2023

reta commented Jan 13, 2023

andrross commented Jan 13, 2023

reta commented Jan 13, 2023

andrross left a comment

dlvenable commented Jan 13, 2023

dlvenable commented Jan 13, 2023

andrross commented Jan 13, 2023

dblock commented Jan 13, 2023

reta commented Jan 13, 2023

[FEATURE] Introduce new OpenSearchTransport based on Apache HttpClient 5 #281

[FEATURE] Introduce new OpenSearchTransport based on Apache HttpClient 5 #281

Conversation

reta commented Nov 25, 2022 • edited Loading

Description

Issues Resolved

reta commented Nov 25, 2022

dblock left a comment

Choose a reason for hiding this comment

reta commented Nov 28, 2022 • edited Loading

reta commented Dec 1, 2022

andrross commented Dec 2, 2022

reta commented Jan 11, 2023

dblock left a comment

Choose a reason for hiding this comment

dblock Jan 12, 2023

Choose a reason for hiding this comment

reta Jan 12, 2023

Choose a reason for hiding this comment

reta Jan 13, 2023

Choose a reason for hiding this comment

dblock Jan 12, 2023

Choose a reason for hiding this comment

reta Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

reta commented Jan 12, 2023

dblock left a comment

Choose a reason for hiding this comment

andrross commented Jan 12, 2023

reta commented Jan 13, 2023

dblock commented Jan 13, 2023 • edited Loading

reta commented Jan 13, 2023

andrross commented Jan 13, 2023

reta commented Jan 13, 2023

andrross commented Jan 13, 2023

reta commented Jan 13, 2023

andrross left a comment

Choose a reason for hiding this comment

dlvenable commented Jan 13, 2023

dlvenable commented Jan 13, 2023

andrross commented Jan 13, 2023

dblock commented Jan 13, 2023

reta commented Jan 13, 2023

reta commented Nov 25, 2022 •

edited

Loading

reta commented Nov 28, 2022 •

edited

Loading

reta Jan 12, 2023 •

edited

Loading

dblock commented Jan 13, 2023 •

edited

Loading