ci(native): Add container tests for sidecar#25200
ci(native): Add container tests for sidecar#25200anandamideShakyan wants to merge 21 commits intoprestodb:masterfrom
Conversation
f5f4c2f to
085a9bd
Compare
c1de62c to
80e1408
Compare
...c/test/java/com/facebook/presto/nativeworker/ContainerNativeQueryRunnerWithSidecarUtils.java
Outdated
Show resolved
Hide resolved
80e1408 to
39c9a0c
Compare
pdabre12
left a comment
There was a problem hiding this comment.
@anandamideShakyan Thanks for this code, have some initial comments.
...to-native-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunner.java
Outdated
Show resolved
Hide resolved
...to-native-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunner.java
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Show resolved
Hide resolved
37e938d to
c2e1160
Compare
c2e1160 to
a0f5fb8
Compare
...to-native-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunner.java
Outdated
Show resolved
Hide resolved
a0f5fb8 to
46a9607
Compare
|
@anandamideShakyan I think we can go about it like this:
CC: @pdabre12 what do you think? |
|
@tdcmeehan Great idea ! |
|
@pdabre12 @tdcmeehan I added the check for ClusterSizeMonitor.hasRequiredCoordinatorSidecar and setup polling for v1/info/state. The query still fails even after the state is "ACTIVE". I have pushed the code with the changes, anything I might be missing here ? |
02e35d3 to
fa71375
Compare
|
I was able to run and verify that all the test cases are passing. |
There was a problem hiding this comment.
Thanks @anandamideShakyan.
Can you please post a screenshot of the passing tests?
...to-native-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunner.java
Show resolved
Hide resolved
...to-native-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunner.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...tive-execution/src/test/java/com/facebook/presto/nativeworker/ContainerQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...src/test/java/com/facebook/presto/nativeworker/TestPrestoContainerSidecarInfrastructure.java
Outdated
Show resolved
Hide resolved
| public void TestNativeClusterWithDelayedSidecar() | ||
| throws Exception | ||
| { | ||
| ContainerQueryRunner queryRunner = new ContainerQueryRunner(4, true, true, true); |
There was a problem hiding this comment.
I don't think it's needed to check for delayed sidecar here. We can assume that the necessary conditions were satisfied before attempting to run a query.
WDTY @tdcmeehan ?
There was a problem hiding this comment.
I will remove the delayedsidecar testcase if it is not needed. @tdcmeehan can you confirm ?
187dbb6 to
07408b7
Compare
czentgr
left a comment
There was a problem hiding this comment.
Looks good but a few things need to be addressed.
| cd presto-native-execution | ||
| make velox-submodule | ||
|
|
||
| - name: Enable Docker BuildKit (daemon) |
There was a problem hiding this comment.
You have to install the BuildKit through a workflow like this
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@e468171a9de216ec08956ac3ada2f0791b6bd435 # v3.1
After that you should be good and you don't need this step.
There was a problem hiding this comment.
@czentgr I added this in the workflow, also added env: DOCKER_BUILDKIT: 1 but still it is failing. Am I missing anything here ?
| # ----------------------------- | ||
| jobs: | ||
| build-worker-image: | ||
| runs-on: ubuntu-22.04 |
There was a problem hiding this comment.
Use ubuntu-latest here and for the other jobs.
| -Denable.docker.build.worker=true \ | ||
| -pl '!:presto-product-tests,!:presto-router,!:presto-docs,!:presto-verifier,!:presto-test-coverage' | ||
|
|
||
| - name: Prune docker build cache |
There was a problem hiding this comment.
Why are we doing this? This container is cleared anyway. We don't have to worry about disk space.
Once we have build the docker images - where are they going? They are currently local on this machine. Each job is a self contained entity. Artifacts don't cross job boundaries. You will have to export the docker image and store it (upload artifact) and then download it in the actual test job.
presto-native-execution/pom.xml
Outdated
| <scope>runtime</scope> | ||
| </dependency> | ||
| </dependencies> | ||
| <!-- <dependencies>--> |
There was a problem hiding this comment.
Yes, I will remove it. It was throwing circular dependency error, so commented out this part and tried another approach.
2295a00 to
9f6deb2
Compare
9f6deb2 to
3162668
Compare
Description
Implement container based test cases for Java/native clusters.
Test matrix: Native cluster
Test Matrix :Java cluster
Contributor checklist
Release Notes
If release note is NOT required, use: