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.

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

HIVE-28104. (2.3) Move HTTP related methods from Utils to HttpUtils in shims #5114

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

pan3793
Copy link
Member

@pan3793 pan3793 commented Mar 3, 2024

What changes were proposed in this pull request?

HIVE-13853 added some HTTP-related methods to the shims Utils, this PR aims to move it to a dedicated class HttpUtils

Why are the changes needed?

The invocation of any method in Utils will trigger all imported classes to be loaded, which blocks the downstream projects from migrating from javax.servlet-api to jakarta.servlet-api, see more context at apache/spark#45154 (comment)

Does this PR introduce any user-facing change?

No.

Is the change a dependency upgrade?

No.

How was this patch tested?

Pass CI(no new failed tests) to ensure everything goes well on the Hive side.

Spark integration tests

@pan3793
Copy link
Member Author

pan3793 commented Mar 4, 2024

* Return Hadoop-native RestCsrfPreventionFilter if it is available.
* Otherwise, construct our own copy of its logic.
*/
public static Filter getXSRFFilter() {
Copy link
Member Author

@pan3793 pan3793 Mar 5, 2024

Choose a reason for hiding this comment

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

I'm not sure if the Hive community treats it as a public API, if there are concerns about compatibility, another approach is:

  1. leaving Utils as-is
  2. creating a HadoopUtils, copying all other methods to HadoopUtils
  3. changing callers to use HadoopUtils instead of Utils to avoid initializing Utils

Choose a reason for hiding this comment

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

Sounds like a more compatible solution.

Copy link
Member

Choose a reason for hiding this comment

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

Either works for me. Looks like this method is only in one place.

@sunchao sunchao merged commit 5ba87ba into apache:branch-2.3 Mar 18, 2024
1 check failed
@sunchao
Copy link
Member

sunchao commented Mar 18, 2024

Merged, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants