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

[FLINK-27160] Add e2e tests for session job #179

Merged
merged 3 commits into from
Apr 25, 2022

Conversation

Aitozi
Copy link

@Aitozi Aitozi commented Apr 24, 2022

This PR is meant to add the e2e test for the session job. It mostly aligned to the flinkdep tests.

@Aitozi
Copy link
Author

Aitozi commented Apr 24, 2022

cc @wangyang0918 @gyfora

@Aitozi
Copy link
Author

Aitozi commented Apr 24, 2022

FYI, I found that the test can not completely run on the Mac now. Because the minikube ip can not be reached from the local machine.

@wangyang0918
Copy link
Contributor

The result of minikube ip should be accessible from local machine. What's driver of minikube? I am using hyperkit and it works well.

@Aitozi
Copy link
Author

Aitozi commented Apr 24, 2022

The result of minikube ip should be accessible from local machine. What's driver of minikube? I am using hyperkit and it works well.

I use the Docker as the driver. It seems due to this: kubernetes/minikube#11193 (comment)

Copy link
Contributor

@wangyang0918 wangyang0918 left a comment

Choose a reason for hiding this comment

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

I left some minor comments.

@@ -47,6 +48,7 @@ function wait_for_jobmanager_running() {
function assert_available_slots() {
expected=$1
ip=$(minikube ip)
curl http://$ip/default/${CLUSTER_ID}/overview
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this line?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 41 to 49
retry_times 30 3 "kubectl get deploy/${CLUSTER_ID}" || exit 1

kubectl wait --for=condition=Available --timeout=${TIMEOUT}s deploy/${CLUSTER_ID} || exit 1
jm_pod_name=$(kubectl get pods --selector="app=${CLUSTER_ID},component=jobmanager" -o jsonpath='{..metadata.name}')

echo "Waiting for jobmanager pod ${jm_pod_name} ready."
kubectl wait --for=condition=Ready --timeout=${TIMEOUT}s pod/$jm_pod_name || exit 1

wait_for_logs $jm_pod_name "Rest endpoint listening at" ${TIMEOUT} || exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be deduplicated by having wait_for_jobmanager_running in utils.sh.

SESSION_CLUSTER_IDENTIFIER="flinkdep/session-cluster-1"
SESSION_JOB_IDENTIFIER="sessionjob/flink-example-statemachine"

function cleanup_and_exit() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be extracted to util.sh.

wait_for_logs $jm_pod_name "Rest endpoint listening at" ${TIMEOUT} || exit 1
}

function assert_available_slots() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be extracted to util.sh and deduplicated.

@Aitozi
Copy link
Author

Aitozi commented Apr 24, 2022

I have addressed your comments, please take a look again @wangyang0918

Copy link
Contributor

@wangyang0918 wangyang0918 left a comment

Choose a reason for hiding this comment

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

+1 for merging.

@wangyang0918 wangyang0918 merged commit 87062b8 into apache:main Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants