Skip to content

HIVE-29047: Adjust httpclient5 httpcore dependencies in TestHttpServices#5907

Merged
okumin merged 1 commit into
apache:masterfrom
richardantal:HIVE-29047
Jul 2, 2025
Merged

HIVE-29047: Adjust httpclient5 httpcore dependencies in TestHttpServices#5907
okumin merged 1 commit into
apache:masterfrom
richardantal:HIVE-29047

Conversation

@richardantal

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Adjust httpclient5 httpcore dependencies in TestHttpServices
https://github.com/richardantal/hive/pull/new/HIVE-29047

Why are the changes needed?

In TestHttpServices we use transitive httpclient5 and httpcore5 dependencies because of
174ac62
Later It will break in case of 1.26 avatica version

Does this PR introduce any user-facing change?

No.

How was this patch tested?

I ran TestHttpServices unit test

@okumin okumin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

Comment thread iceberg/pom.xml Outdated
<groupId>org.apache.httpcomponents.core5</groupId>
<artifactId>httpcore5</artifactId>
</exclusion>
</exclusions>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What happens if we don't exclude it? We eventually need this dependency when we adapt Hive QL to Iceberg REST API. It sounds fine to exclude it at this point.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It works without the exclusions too.

@richardantal

Copy link
Copy Markdown
Contributor Author

I rebased the change on top of master

@richardantal

Copy link
Copy Markdown
Contributor Author

Do we need an other clean test run or how can we move forward with this?

@okumin

okumin commented Jul 1, 2025

Copy link
Copy Markdown
Contributor

@richardantal
I periodically check the CI status and rerun it if it fails. Therefore, there are no remaining tasks on your end. The GitHub Actions error is likely caused by the following issue, which will be resolved soon. I will rerun the task in a few hours. Thank you!

https://lists.apache.org/thread/qbfh1fq6rj629y5xrt45v0qw64828zf2

@abstractdog

Copy link
Copy Markdown
Contributor

@richardantal : thanks for this patch! please ban http components 5 import to prevent future unintentional usage
if that's taken care of, I'm fine with the patch!

@abstractdog abstractdog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, pending tests

@sonarqubecloud

sonarqubecloud Bot commented Jul 1, 2025

Copy link
Copy Markdown

@okumin
okumin merged commit 4bdd0f1 into apache:master Jul 2, 2025
6 checks passed
@okumin

okumin commented Jul 2, 2025

Copy link
Copy Markdown
Contributor

Merged. @richardantal Thanks for submitting the patch, @abstractdog thanks for the additional review!

@richardantal

Copy link
Copy Markdown
Contributor Author

Thank you for merging, and for the reviews.

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.

4 participants