[#1247] Improvement (trino-connector) Make Trino QueryIT test cases running in ci. #1262
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
diqiu50
force-pushed
the
trino-connector-ut
branch
from
42fcc4c to
c812d4b
Compare
diqiu50
force-pushed
the
trino-connector-ut
branch
from
c812d4b to
2bb4c03
Compare
diqiu50
force-pushed
the
trino-connector-ut
branch
from
16254db to
2f2cc50
Compare
diqiu50
force-pushed
the
trino-connector-ut
branch
from
79ac1e4 to
6e1b195
Compare
diqiu50
force-pushed
the
trino-connector-ut
branch
from
6e1b195 to
4ad5638
Compare
xunliu
reviewed
Jan 4, 2024
...est/src/test/java/com/datastrato/gravitino/integration/test/container/TrinoITContainers.java
Outdated
Show resolved
Hide resolved
...gration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoQueryIT.java
Outdated
Show resolved
Hide resolved
...gration-test/src/test/java/com/datastrato/gravitino/integration/test/trino/TrinoQueryIT.java
Outdated
Show resolved
Hide resolved
| @@ -6,3 +6,4 @@ | |||
| # Fixed docker container network subnet in Gravitino integration testing module | |||
| # Generate command: `docker network ls --filter driver=bridge --format "{{.ID}}" | xargs docker network inspect --format "route {{range .IPAM.Config}}{{.Subnet}}{{end}}" > ${bin}/docker-connector.conf` | |||
| route 10.20.30.0/28 | |||
| route 10.20.30.16/28 | |||
There was a problem hiding this comment.
Why do we need to change this route configuration?
There was a problem hiding this comment.
Docker Compose uses the 10.20.30.16/28 network. Because if you use the integrated testing network, manually starting Docker Compose will result in errors due to the lack of network.
Using ITUtils.joinPath to concat file name.
|
@jerryshao Please take a review. |
…t on Trino image gravitino-ci-trino:0.1.3. (apache#1329) ### What changes were proposed in this pull request? Fix the failed 'drop catalog' test on Trino image gravitino-ci-trino:0.1.3. Trino image gravitino-ci-trino:0.1.2 use the gravitino-trino-connector-0.3.0-SNAPSHOT.jar, it does not support drop catalog, so the tester has made it compatible with this. Trino image gravitino-ci-trino:0.1.3 use the gravitino-trino-connector-0.4.0-SNAPSHOT.jar, it support drop catalog. the test did not pass. ### Why are the changes needed? Fix: apache#1328 ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? NO
…crement interface. (apache#1278) ### What changes were proposed in this pull request? Add column attribute autoIncrement of the increment interface. ### Why are the changes needed? Fix: apache#1277 ### Does this PR introduce _any_ user-facing change? 1. Change in user-facing APIs. ### How was this patch tested? UT Co-authored-by: Clearvive <[email protected]>
… allowMethods for CorsFilter (apache#1348) ### What changes were proposed in this pull request? Modify the default value of allowMethods for CorsFilter ### Why are the changes needed? Our alter api uses the method `PUT`. It will be more convenient for users. ### Does this PR introduce _any_ user-facing change? Modify the document. ### How was this patch tested? By hand. Co-authored-by: Heng Qin <[email protected]> Co-authored-by: Jerry Shao <[email protected]>
### What changes were proposed in this pull request? add missing headers and license ### Why are the changes needed? Fix: apache#1267 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? N/A (no code change, only add missing license)
What changes were proposed in this pull request? Add badges to README file Why are the changes needed? Fix: apache#1346 Does this PR introduce any user-facing change? No How was this patch tested? No need.
|
Let's follow the rule that only newly added files need to update the license year @diqiu50 . |
jerryshao
reviewed
Jan 8, 2024
|
|
||
| String containerIpMapping = output.toString(); | ||
| if (containerIpMapping.isEmpty()) { | ||
| throw new Exception("Missing to get container's ip"); |
There was a problem hiding this comment.
"Missing to get container's ip", it's difficult to understand the meaning.
jerryshao
reviewed
Jan 8, 2024
| } | ||
| } | ||
| } catch (Exception e) { | ||
| throw new Exception("Failed to parse container ip:\n" + containerIpMapping, e); |
There was a problem hiding this comment.
Can we use a specific Exception here and below?
jerryshao
approved these changes
Jan 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Make Trino QueryIT test cases running in ci.
To run Trino Query IT, a complete test environment including Hive, Iceberg, PostgreSQL, and MySQL dependencies is required. We need to set up this test environment and execute the test cases automatically.
Why are the changes needed?
Fix: #1247 #919
Does this PR introduce any user-facing change?
NO
How was this patch tested?
NO