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

[SPARK-46938][BUILD] Remove javax-servlet-api exclusion rule for SBT #45194

Closed

Conversation

HiuKwok
Copy link
Contributor

@HiuKwok HiuKwok commented Feb 21, 2024

What changes were proposed in this pull request?

  • Update SBT build file to remove the exclusion rule for javax-servlet-api package.

Why are the changes needed?

Does this PR introduce any user-facing change?

No

How was this patch tested?

CI build

Was this patch authored or co-authored using generative AI tooling?

No

@github-actions github-actions bot added the BUILD label Feb 21, 2024
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Oh, this PR is unable to pass CI, @HiuKwok ? I guess this PR should pass CIs and the previous PR will cause failure without this. Did I understand correctly?

this will cause test failure,

@HiuKwok
Copy link
Contributor Author

HiuKwok commented Feb 21, 2024

Oh, this PR is unable to pass CI, @HiuKwok ?

this will cause test failure,

It will only cause failure when Jetty upgrades to 11 AND the exclusion rule is present.
By simply removing the rule won't fail the build I belive.

@HiuKwok
Copy link
Contributor Author

HiuKwok commented Feb 21, 2024

@dongjoon-hyun Your understanding is correct 👍

@dongjoon-hyun
Copy link
Member

Could you revise the PR description? You don't need to mention any failure here. It only causes confusions.

@HiuKwok
Copy link
Contributor Author

HiuKwok commented Feb 21, 2024

Done.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Let's wait and see the CI result because this is a tricky dependency change in terms of set compilation error.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-46938][BUILD][CORE] Remove javax-servlet-api exclusion rule for SBT [SPARK-46938][BUILD] Remove javax-servlet-api exclusion rule for SBT Feb 21, 2024
@dongjoon-hyun dongjoon-hyun changed the title [SPARK-46938][BUILD] Remove javax-servlet-api exclusion rule for SBT [SPARK-46938][BUILD] Remove javax-servlet-api exclusion rule for SBT Feb 21, 2024
@@ -1075,7 +1075,6 @@ object ExcludedDependencies {
// purpose only. Here we exclude them from the whole project scope and add them w/ yarn only.
excludeDependencies ++= Seq(
ExclusionRule(organization = "com.sun.jersey"),
ExclusionRule("javax.servlet", "javax.servlet-api"),
Copy link
Member

Choose a reason for hiding this comment

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

this is intended, to avoid conflict with jakarta.servlet-api:4.x, the tricky thing is that jakarta.servlet-api:4.x actually contains javax.servlet classes

Copy link
Member

@dongjoon-hyun dongjoon-hyun Feb 21, 2024

Choose a reason for hiding this comment

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

Ya, this is a spin-off from the following original PR. We know that this is a transient status, @pan3793 . Do you have any other concern?

Copy link
Member

Choose a reason for hiding this comment

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

I get the point. The change makes sense to me.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM as a preparation for the following original PR. Pending CIs.

@dongjoon-hyun
Copy link
Member

Thank you, @HiuKwok and @pan3793 .
Merged to master.