Conversation
b0cf998 to
0f7e32c
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
0f7e32c to
1a902a0
Compare
1a902a0 to
eb3162e
Compare
eb3162e to
24c1805
Compare
f33bd66 to
24c1805
Compare
|
Looks like Spark tests are failing because Spark internally still depends on an older Jetty version: |
|
Can the jetty depended by Iceberg tests be separated from that by Spark? I don't think iceberg-spark-runtime needs to include jetty dependency from Iceberg. |
|
@nastra Are you still working on this? |
I'm not actively working on this right now |
|
@nastra I can pick this up if you don't have time currently. |
|
@manuzhang sure, just go ahead and open a separate PR |
6b15c1a to
c9d14a6
Compare
|
@manuzhang #15232 contained too many unrelated changes and was also missing a bunch of other places where the UI config had to be disabled. However, the idea of disabling the UI is correct and I've added those here and added you as the author. @singhpk234 @amogh-jahagirdar we should merge the PR using "Rebase and merge" so that the individual commits get merged separately |
| CreateMultipartUploadRequest.builder().bucket(BUCKET).key("random/multipart-key").build()); | ||
| } | ||
|
|
||
| @SuppressWarnings("removal") |
There was a problem hiding this comment.
this is needed because GzipHandler is now deprecated. We'll be switching to Jetty's new Compression API in #15043
| this.spark = | ||
| SparkSession.builder() | ||
| .config("spark.ui.enabled", false) | ||
| .config(TestBase.DISABLE_UI) |
There was a problem hiding this comment.
The main issue with Spark is that Spark brings its own (older) Jetty version for the UI, which conflicts with the Jetty version we're using. The downside is that we need to disable the UI pretty much everywhere
There was a problem hiding this comment.
Spark only upgraded to jetty 12 after 4.2, so I believe it's worthwhile. As a follow-up, I plan to create a util method to start SparkSession such that we don't need to copy the same config everywhere.
There was a problem hiding this comment.
Does this mean it's going to be an issue for downstream users? If not why don't we switch these benchmarks to use the fat jar?
c9a3108 to
49cdaf1
Compare
|
@singhpk234 @amogh-jahagirdar can you guys please review this one? |
49cdaf1 to
b4e9819
Compare
| testImplementation project(path: ':iceberg-hive-metastore', configuration: 'testArtifacts') | ||
| testImplementation project(path: ":iceberg-spark:iceberg-spark-${sparkMajorVersion}_${scalaVersion}", configuration: 'testArtifacts') | ||
| testImplementation (project(path: ':iceberg-open-api', configuration: 'testFixturesRuntimeElements')) { | ||
| transitive = false |
There was a problem hiding this comment.
this was initially done because there were conflicts. However, given that we don't have any conflicts anymore we can remove this
|
@nastra I prefer to merge this in one commit since the first commit doesn't pass test and we can't revert the commits separately. |
|
@nastra can you please rebase this PR? |
9002df6 to
a025f53
Compare
RussellSpitzer
left a comment
There was a problem hiding this comment.
I would hope we could get away without disabling the UI, this seems like a bit of a code smell. If our builds can't handle multiple versions are we sure this is going to work with end users of the fat jar/ using their own packages?
I don't often use the UI for debugging tests but it feels like we need to make sure we aren't just masking what would be a downstream issue.
RussellSpitzer
left a comment
There was a problem hiding this comment.
Ok I looked closer, I see that we are really just replacing a constant. We were disabling the UI before in most cases anyway.
|
Yeah sorry for the late review @nastra , I checked this out last week and had the same realization as @RussellSpitzer above that the UI was disabled anyways, I think it looks good to me |
With the new Jetty upgrade in apache/iceberg#10837 we're running into the following: Failing CI: https://github.com/apache/iceberg-python/actions/runs/24416422109/job/71326697962 ```json Malformed request: { "message":"Suspicious Path Character", "url":"http://rest:8181/v1/namespaces/default%1Ftest_positional_mor_deletes_v2/tables/branch_without_5?snapshots=all", "status":"400" } ``` The `namespace-seperator` has been introduced afther the 1.10.x branch (apache#2826), so it isn't out in public, meaning the Spark integration tests are not respecting this config. This requires us to downgrade for now. I think disallowing the default (`%1F`) separator is problematic, since it might break compatibility.
Now that we fixed #10338 and moved to JDK 17 we're finally able to upgrade Jetty and the Servlet API to the latest version