Azure / azure-kusto-java

Microsoft Azure Kusto Library for Java
MIT License
38 stars 43 forks source link

Potential removal of any remaining Apache http client components #396

Closed georgebanasios closed 1 week ago

georgebanasios commented 2 weeks ago

Is your feature request related to a problem? Please describe. Not related to a problem, more of a thought for an enhancement, after seeing the changes regarding the removal of the Apache HTTP client and the move to the azure core client instead. As Cole (@The-Funk) mentioned in his PR, if I'm not mistaken, the only component used from the old Apache client is the URIBuilder object, which it could be removed with a slight change on the implementation. I'd like to ask for permission to submit a PR in order to remove both apache http client and http core dependencies from data and ingest modules.

Correction: I'm trying to determine if the two stream wrappers on the streamingQuery are still needed with the new http client and hence, an upgrade to http core v5 might be needed or if they can be completely removed. What I mentioned about URIBuilder still applies.

Describe the solution you'd like After reviewing the implementations of URIBuilder (apache http) and URI (java net) objects, I noticed that what's causing the issue when removing the URIBuilder and relying only on the URI object, is that the URI object does not recognize the IPv6 address format (due to the absence of brackets) the first time the parsing of the string cluster URL (directly retrieved from the ConnectionStringBuilder) is happening.

With just one line of change, by determining whether the host component is an IPv6 address (which can be resolved using a core Java method), we can reconstruct the cluster URL from a URI object without the need for URIBuilder. The resolution I mentioned is already being handled in the ingest module by calling a helper method from the Apache http client (same method that URIBuilder uses internally, so we would not be doing anything new/different). However, by determining whether the host component is an IPv6 address using a core Java method, this process can now be done without the Apache dependency. I can submit a PR if you'd like to take a look.

Describe alternatives you've considered Keep the implementation as is for now, if it's not worth addressing in the near future.

yogilad commented 2 weeks ago

Thanks, @georgebanasios. (This is more of an internal note)

The correct way to note IPv6 in addresses is with brackets. Since the change to switch from Apache to Azure Core is breaking as it is, let's just document this behavior change in the release notes.

@georgebanasios , if you still wish to contribute this change (without any detection parsing per above), we'd appreciate it.