-
Notifications
You must be signed in to change notification settings - Fork 319
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
[#3305] test(spark-connector): move spark connector integration test from integration-test module to spark-connector #3307
Conversation
@@ -54,6 +54,7 @@ kafka = "3.4.0" | |||
curator = "2.12.0" | |||
awaitility = "4.2.1" | |||
servlet = "3.1.0" | |||
jodd = "3.5.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TrinoQueryRunner
use this lib
167458a
to
12f53e0
Compare
e4dbff4
to
e58c1d1
Compare
2a7c373
to
ffec87a
Compare
a1e1447
to
0f4d9a9
Compare
This is blocked by #3441 , there are two ways to bypass:
I prefer option2 is no better solutions. @jerryshao , @qqqttt123 @yuqi1129 @diqiu50 @mchades , WDYT? |
Use solution 2 for now. |
ok |
d04f470
to
b24aa23
Compare
@yuqi1129 @jerryshao could you help to review this PR, thanks |
dependsOn(tasks.jar) | ||
|
||
doFirst { | ||
environment("GRAVITINO_CI_HIVE_DOCKER_IMAGE", "datastrato/gravitino-ci-hive:0.1.10") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be 0.1.11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remembered we have a 0.1.12 version? @yuqi1129
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@yuqi1129 do you have any other comment, @jerryshao do you want to review this PR? |
implementation(project(":clients:client-java-runtime", configuration = "shadow")) | ||
implementation(project(":spark-connector:spark-connector")) | ||
|
||
implementation("org.apache.iceberg:iceberg-spark-runtime-${sparkMajorVersion}_$scalaVersion:$icebergVersion") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to package the iceberg runtime jar into spark connector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'd better not package Iceberg runtime jar, but there are some limits now, we now register Iceberg extensions on startup. I prefer to do it in #3396
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a big work? I think it should be fixed in this version, we cannot ship a Spark connector with iceberg runtime packaged in, it will introduce lots of issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a big work, but we have to keep consistent about this,
- if not registering Iceberg extensions, Iceberg catalogs couldn't be dymatic loaded in the future
- when not registering Iceberg extensions, I prefer to add a configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix this before 0.5.1 is shipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@jerryshao @yuqi1129 comments are addressed, please help to review again |
I have no further comments. |
…from integration-test module to spark-connector (#3307) ### What changes were proposed in this pull request? move spark connector integration test from integration-test module to spark-connector ### Why are the changes needed? Fix: #3305 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? moving test location
… test from integration-test module to spark-connector (apache#3307) move spark connector integration test from integration-test module to spark-connector Fix: apache#3305 no moving test location
… test from integration-test module to spark-connector (apache#3307) ### What changes were proposed in this pull request? move spark connector integration test from integration-test module to spark-connector ### Why are the changes needed? Fix: apache#3305 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? moving test location
What changes were proposed in this pull request?
move spark connector integration test from integration-test module to spark-connector
Why are the changes needed?
Fix: #3305
Does this PR introduce any user-facing change?
no
How was this patch tested?
moving test location