[FLINK-8985][e2etest] initial support for End-to-end CLI test, excluding YARN test #5863
Conversation
walterddr
force-pushed
the
FLINK-8985
branch
2 times, most recently
from
d65157f to
ea12737
Compare
There was a problem hiding this comment.
Thank a lot for the PR @walterddr! And sorry for the long delay on the review.
Overall, this looks quite good. I have some comments to address before we merge this.
| printf "Test run with complex parameter set\n" | ||
| printf "==============================================================================\n" | ||
| if [ $EXIT_CODE == 0 ]; then | ||
| eval "$FLINK_DIR/bin/flink run -m localhost:8081 -p 4 -q -d \ |
There was a problem hiding this comment.
There probably should be some verification that the job actually runs with DOP=4
There was a problem hiding this comment.
Since this is a detached execution, we probably want to wait until this job completes before continuing?
There was a problem hiding this comment.
Changed to use -p 1. I think the part that "parallelism should be taken by the CLI command" is verified by the exit code from the run execution. is that correct?
There was a problem hiding this comment.
Well, we can have a completely normal exit code from the run execution, but the -p option completely ignored if we change the CLI to simply not recognize the option.
This is an extreme case, though.
There was a problem hiding this comment.
good point. I will add the check 👍
| printf "Test information APIs\n" | ||
| printf "==============================================================================\n" | ||
| if [ $EXIT_CODE == 0 ]; then | ||
| eval "$FLINK_DIR/bin/flink info $FLINK_DIR/examples/batch/WordCount.jar" |
There was a problem hiding this comment.
Should we verify the output of info?
| EXIT_CODE=$? | ||
| fi | ||
| if [ $EXIT_CODE == 0 ]; then | ||
| eval "$FLINK_DIR/bin/flink list" |
There was a problem hiding this comment.
Should we verify the output of list?
| fi | ||
|
|
||
| printf "\n==============================================================================\n" | ||
| printf "Test savepoint for a running streaming jobs\n" |
There was a problem hiding this comment.
I think this part can be removed. The coverage is already subsumed by test_resume_savepoint.sh.
What do you think?
|
Thanks @tzulitai for the review. I will update asap. |
|
|
||
| function cleanup_cli_test() { | ||
| stop_cluster | ||
| $FLINK_DIR/bin/taskmanager.sh stop-all |
There was a problem hiding this comment.
I don't think we need to explicitly shutdown the cluster and TMs here; that is already part of the cleanup call
There was a problem hiding this comment.
checking the clean up code seems like it doesn't explicitly call stopping all tm (similar to start-cluster.sh, I need to explicitly call taskmanager start). I should remove only the stop-cluster actually.
|
Thanks for the PR @walterddr. I think the test looks good to merge. I will add some additional safety checks and rebase this code to the current master. |
What is the purpose of the change
Adding end to end test for CLI APIs.
Brief change log
Added test_cli_api.sh script to test combinations of CLI commands listed in the doc section of Flink. Including:
Verifying this change
This is a test
Does this pull request potentially affect one of the following parts:
No
Documentation
No