Removes apache http libraries and replace with azure core httpclient #335

The-Funk · 2023-12-10T21:19:13Z

This PR is ready for review. Updated Notes 3/2/2024.

Notes:

The removal of the entity stuff was difficult as expected. There was a lot of use of Entity and a lot of use of the URIBuilder from Apache core. I simplified this to some extent and I'd like to open a new PR in the future to remove the usage of URIBuilder completely.
The old Apache Client comes with an exception called ParseException that was being used to indicate parsing failures. The version of said exception from the V4 client library, extends RuntimeException. To replace it I created a KustoParseException that extends RuntimeException for the sake of compatibility. This should work as a drop in replacement. After refactoring, this class is only used in TESTS now.
Rather than using the Netty HttpClient implementation from core, I used the HttpClient interface itself. This way, if someone wants to replace the Netty client they can do so easily by adding a different classname to their GlobalConfig or excluding the Netty client jar from their dependencies and replacing it with their own jar. For instance, I have a personal interest in utilizing the azure-core-http-vertx client once it comes out of Beta. The vertx client better supports native image building with GraalVM. The Netty client implementation is the default implementation included with azure core. We should avoid using anything with Netty specifically in the name in order to avoid becoming locked to the Netty client.
Moved all request building logic (some from HttpPostUtils and some from ClientImpl) to a request builder class in order to provide better testability and separation of concerns and reduce the number of parameters that need passed between methods.
Moved all HTTP Request Tracing parameters to a specific builder class for better testability, separation of concerns, etc, same as above.
Moved remainder of HttpPostUtils into a BaseClient class that the ClientImpl extends, removing the need to pass the HTTP client to a separate class and promoting long lived HTTP Clients.
I deleted the HttpResponseWrapper. Since the Apache client is removed, this class should no longer be needed.

BREAKING CHANGES:

In the ClientRequestProperties class the proxy option used HttpHost from the Apache libraries directly. This is exposed directly to the users of the library, so in order to remove the Apache dependencies I needed to remove HttpHost. I replaced this with the ProxyOptions from Azure core but this would be a breaking change for users of the library that specify a custom proxy. They would need to rework a few lines of code, no more than 3 or 4 max.
HttpClientFactory now accepts any client implementing azure-core's HttpClient interface instead of an Apache CloseableHttpClient.

In Tests:

The UtilitiesTest in the data module made use of BasicHttpResponse from the Apache libraries. Because the HTTPResponse class from Azure core is an abstract class, creating new instances of it manually is not simple. This is probably by design. But in the event that we want to create a fake HTTPResponse, for instance in tests, we need a way to do so. There is a utility class in Tests now that will allow for the creation of fake HttpResponses. I created a simple implementation for it including only what had been used previously, and a builder to make it easier to instantiate new instances.
CloudInfo tests now use the RequestBuilder

…lient.

Removed cast to Closeable in CloudInfo

ohadbitt · 2023-12-13T14:08:02Z

Hey
Sounds like a very well thought process really like the usage of HttpClient - looks like a great fit.

Reactor is pretty much the same as mutiny (not familiar with the latter but it looks pretty similar approach ) but i could help with overcoming any problems you got trying to use it - you intend that we overcome the Todos in this PR ? As my intentions were that the Async part will be handled in the next PR and now only have simple sync impl - please tell me which part would you like me to take in this PR i would do it first thing this week or the next.
ProxyOptions is a very good implementation that we want anyway.
Regarding closeable - IngestClient has to be closeable, and only the STreamingIngestClient will have it as a noOp - implemented this part now while looking at the code

The-Funk · 2023-12-13T14:28:32Z

@ohadbitt

You're right about the closeable.

I like your plan of a separate PR to work on the async implementation. That's a lot more changes.

There's also one or two other design things that I think might make sense while we're performing surgery around the HTTP Clients. I think it would be nice to get rid of HttpPostUtils and move the post methods onto an abstract class that all of the KustoClients should extend. That way the enclosed HTTP client can be a protected property of the parent abstract class, as well as the post methods. That would mean less passing around of the HTTP client, meaning shorter method signatures. I saw there was also a comment about making the base URL string a class property that can be set one time upon KustoClient creation and then reused. I liked that idea as well.

ohadbitt · 2023-12-13T15:34:19Z

@The-Funk Sounds good - maybe better @AsafMah will look as well at all the planning

AsafMah · 2023-12-14T09:11:20Z

I'm not sure about KustoParseException extending RuntimeException, don't we want our exceptions to be checked?

ohadbitt · 2023-12-14T11:37:24Z

I too am against RuntimeExceptions - @The-Funk , sounds like you should go ahead with the other changes, when ready hit the "ready for review"

The-Funk · 2023-12-14T13:48:56Z

Speaking of unchecked exceptions, I had extra throws for that exception that I didn't need, I just pushed that.

In this case, I think KustoParseException made sense because the original ParseException that needed replaced was unchecked, so to get the same behavior without changing the method signatures, I replaced it with a new unchecked exception.

If you would like I could replace it with the new Apache HTTP core's ParseException which is a checked exception in V5.

Setting ready for review now.

ohadbitt

Went through it all. Main issue is testing this properly, we need some long query, big data, multi threaded stress tests to really know we are ok here as we never had such a change. Will you take this as well ?

ohadbitt · 2023-12-19T18:24:23Z

data/src/main/java/com/microsoft/azure/kusto/data/auth/CloudInfo.java

                    if (errorFromResponse.isEmpty()) {
-                        errorFromResponse = response.getStatusLine().getReasonPhrase();
+                        // Fixme: Missing reason phrase to add to exception. Potentially want to use an enum.


should be part of the response - if its not i don't think we have to much to add

data/src/main/java/com/microsoft/azure/kusto/data/auth/CloudInfo.java

data/src/main/java/com/microsoft/azure/kusto/data/auth/HttpClientWrapper.java

data/src/main/java/com/microsoft/azure/kusto/data/exceptions/KustoParseException.java

data/src/main/java/com/microsoft/azure/kusto/data/http/HttpClientFactory.java

data/src/main/java/com/microsoft/azure/kusto/data/http/HttpPostUtils.java

ohadbitt · 2023-12-20T11:10:59Z

ingest/src/main/java/com/microsoft/azure/kusto/ingest/ResourceManager.java

@@ -67,7 +65,7 @@ public class ResourceManager implements Closeable, IngestionResourceManager {
    private IngestionResource<QueueWithSas> successfulIngestionsQueues;
    private IngestionResource<QueueWithSas> failedIngestionsQueues;

-    ResourceManager(Client client, long defaultRefreshTime, long refreshTimeOnFailure, @Nullable CloseableHttpClient httpClient) {
+    ResourceManager(Client client, long defaultRefreshTime, long refreshTimeOnFailure, @Nullable HttpClient httpClient) {
        this.defaultRefreshTime = defaultRefreshTime;
        this.refreshTimeOnFailure = refreshTimeOnFailure;
        this.client = client;


No need the HttpClientWrapper(httpClient) here :)

@ohadbitt you want me to use the HttpClientWrapper here?

ingest/src/main/java/com/microsoft/azure/kusto/ingest/QueuedIngestClientImpl.java

data/src/main/java/com/microsoft/azure/kusto/data/http/HttpPostUtils.java

data/src/main/java/com/microsoft/azure/kusto/data/ClientImpl.java

-Adds custom HTTP status codes -Integrates status codes -Uses Context.NONE static factory

The-Funk · 2023-12-22T21:30:24Z

@ohadbitt For the const values, I created a specific class called HttpStatus in the http package and put const values in it.

The reason is, the other status codes constants come from the Apache library, and if we want to remove even the core library eventually, we'll need to migrate away from the Apache codes.

Those same Apache status codes are provided as a transitive dependency of another dependency in the project (nimbus) but it probably wouldn't be a good idea to rely on them being there since we wouldn't be including the codes directly anymore if we get rid of Apache HttpCore.

…ects in QueuedIngestClientImpl per review comments

The-Funk · 2023-12-22T21:38:15Z

Went through it all. Main issue is testing this properly, we need some long query, big data, multi threaded stress tests to really know we are ok here as we never had such a change. Will you take this as well ?

@ohadbitt

I have a development cluster that I can run some long running queries against. I can also try some test ingestions as well. I usually ingest data as JSON though. The part that worries me is ingestions involving file sources and streams.

For scale I can use JMeter and compare/contrast my service RAM/CPU with the current version of the library.

I don't know if I can give you really good in-depth insights though. That might be beyond me.

removes Entity references

…apper

…ve. updates CHANGELOG.md ran formatter.

The-Funk · 2024-03-02T02:08:21Z

@AsafMah I think I addressed most points. All tests that I can run appear to be passing. I have not loaded in OkHttp but if you would like me to remove Netty and add OkHttp now, I can.

ohadbitt

Lets go :)
@The-Funk ill make them pass?

…-Funk/azure-kusto-java into remove_apache_http_libraries

AsafMah

Let's resolve these and finally close this.

data/src/main/java/com/microsoft/azure/kusto/data/BaseClient.java

data/src/main/java/com/microsoft/azure/kusto/data/http/HttpClientFactory.java

The-Funk · 2024-03-14T00:47:34Z

FYI @AsafMah @ohadbitt I fully expect that with this big of a change there will come challenges and potentially regressions. I am willing to help if issues arise.

AsafMah

Thank you for your patience, approved!

AsafMah · 2024-03-24T12:06:47Z

@The-Funk

What's next from here?

Are you going to work on the async api?

ohadbitt · 2024-03-25T11:57:16Z

Thank you @The-Funk for your contribution,
As Asaf mentioned- you did want to impl the async api as well, if its not relevant anymore do tell :)

The-Funk · 2024-03-25T15:20:09Z

@The-Funk

What's next from here?

Are you going to work on the async api?

Indeed!

…pclient (#335)" This reverts commit 0057e8f

cole added 12 commits December 10, 2023 15:52

initial commit removing apache http libraries

73b9311

updated MSAL wrapper class to include headers in response

9d35246

Added TestHttpResponse class to replace BasicHttpResponse in Tests

cd5a5a5

Added KustoParseException to replace ParseException from old Apache C…

73754c7

…lient.

removed todo regarding ParseException

989019f

removed EofSensorInputStream

981a45c

added fixme to replace EofSensorInputStream

00001b8

Return of the Apache

f7b7ce5

Fixed UtilitiesTest

2f66eb2

Removed cast to Closeable in CloudInfo

return UriBuilder to UriUtils

3a61d30

add proxy options back to HttpClientFactory

18555a9

ran formatter

d7e9b2b

closeable and fix build

7b7efea

fix test

3429dee

format

5fc9830

removed extra throws

0cc1c22

The-Funk marked this pull request as ready for review December 14, 2023 13:49

ohadbitt reviewed Dec 20, 2023

View reviewed changes

AsafMah reviewed Dec 20, 2023

View reviewed changes

data/src/main/java/com/microsoft/azure/kusto/data/ClientImpl.java Outdated Show resolved Hide resolved

Per Ohad review comments:

d0cada0

-Adds custom HTTP status codes -Integrates status codes -Uses Context.NONE static factory

removed caught exceptions that were rethrown with no changes/side eff…

2ab94f3

…ects in QueuedIngestClientImpl per review comments

removes block on body by using BinaryData

245185b

removes Entity references

cole added 7 commits March 1, 2024 18:35

readd the exclusion of commons-codec for security posture

c25960f

adds response timeout and client provider to the client properties wr…

abae5fb

…apper

removes references to Netty

286bbe1

removes extra comment

d9dc27c

adds Keep-Alive back to prevent breaking change for users of Keep-Ali…

3349315

…ve. updates CHANGELOG.md ran formatter.

removes deflate algorithm from encoding header

9370530

revert removing Apache core (URIBuilder depends on utilities in core)

350dc1b

The-Funk requested a review from AsafMah March 2, 2024 02:08

The-Funk and others added 2 commits March 4, 2024 10:17

remove netty from client factory

b17d8a0

Merge branch 'master' into remove_apache_http_libraries

df53501

ohadbitt approved these changes Mar 10, 2024

View reviewed changes

ohbitton and others added 3 commits March 10, 2024 14:49

fix

ad12749

Merge branch 'remove_apache_http_libraries' of https://github.com/The…

b2edfe7

…-Funk/azure-kusto-java into remove_apache_http_libraries

Merge branch 'master' into remove_apache_http_libraries

767c7b5

AsafMah reviewed Mar 11, 2024

View reviewed changes

The-Funk and others added 3 commits March 11, 2024 14:14

removes CloseParentResourcesStream.java

54d1ec5

reusable Gzip logic

5b6e2f7

factory now uses azure-core default http client options

5d3a6ed

The-Funk requested review from AsafMah and ohadbitt March 14, 2024 00:43

The-Funk and others added 2 commits March 16, 2024 20:25

I always forget the formatter

8676c50

fix test

1f7785b

AsafMah approved these changes Mar 20, 2024

View reviewed changes

AsafMah merged commit 0057e8f into Azure:master Mar 24, 2024
3 of 7 checks passed

AsafMah added a commit that referenced this pull request Jun 23, 2024

Revert "Removes apache http libraries and replace with azure core htt…

ee460e0

…pclient (#335)" This reverts commit 0057e8f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes apache http libraries and replace with azure core httpclient #335

Removes apache http libraries and replace with azure core httpclient #335

The-Funk commented Dec 10, 2023 •

edited

Loading

ohadbitt commented Dec 13, 2023 •

edited

Loading

The-Funk commented Dec 13, 2023

ohadbitt commented Dec 13, 2023

AsafMah commented Dec 14, 2023

ohadbitt commented Dec 14, 2023

The-Funk commented Dec 14, 2023 •

edited

Loading

ohadbitt left a comment

ohadbitt Dec 19, 2023

ohadbitt Dec 20, 2023

The-Funk Jan 2, 2024

The-Funk commented Dec 22, 2023 •

edited

Loading

The-Funk commented Dec 22, 2023 •

edited

Loading

The-Funk commented Mar 2, 2024

ohadbitt left a comment •

edited

Loading

AsafMah left a comment

The-Funk commented Mar 14, 2024

AsafMah left a comment

AsafMah commented Mar 24, 2024

ohadbitt commented Mar 25, 2024

The-Funk commented Mar 25, 2024

Removes apache http libraries and replace with azure core httpclient #335

Removes apache http libraries and replace with azure core httpclient #335

Conversation

The-Funk commented Dec 10, 2023 • edited Loading

ohadbitt commented Dec 13, 2023 • edited Loading

The-Funk commented Dec 13, 2023

ohadbitt commented Dec 13, 2023

AsafMah commented Dec 14, 2023

ohadbitt commented Dec 14, 2023

The-Funk commented Dec 14, 2023 • edited Loading

ohadbitt left a comment

Choose a reason for hiding this comment

ohadbitt Dec 19, 2023

Choose a reason for hiding this comment

ohadbitt Dec 20, 2023

Choose a reason for hiding this comment

The-Funk Jan 2, 2024

Choose a reason for hiding this comment

The-Funk commented Dec 22, 2023 • edited Loading

The-Funk commented Dec 22, 2023 • edited Loading

The-Funk commented Mar 2, 2024

ohadbitt left a comment • edited Loading

Choose a reason for hiding this comment

AsafMah left a comment

Choose a reason for hiding this comment

The-Funk commented Mar 14, 2024

AsafMah left a comment

Choose a reason for hiding this comment

AsafMah commented Mar 24, 2024

ohadbitt commented Mar 25, 2024

The-Funk commented Mar 25, 2024

The-Funk commented Dec 10, 2023 •

edited

Loading

ohadbitt commented Dec 13, 2023 •

edited

Loading

The-Funk commented Dec 14, 2023 •

edited

Loading

The-Funk commented Dec 22, 2023 •

edited

Loading

The-Funk commented Dec 22, 2023 •

edited

Loading

ohadbitt left a comment •

edited

Loading