-
Notifications
You must be signed in to change notification settings - Fork 13k
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-15442] Harden the Avro Confluent Schema Registry nightly end-to-end test #10742
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit b542834 (Thu Jan 02 06:04:49 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
CI report:
Bot commandsThe @flinkbot bot supports the following commands:
|
cc @tillrohrmann |
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.
Thanks for opening this PR @KarmaGYZ. I left some comments as I don't fully understand why the changes are hardening the test script. At the moment, I thought that the downloading KAFKA_URL
would be retried. Hence, I fear that your changes might not really have an effect.
@@ -748,7 +748,7 @@ function retry_times_with_backoff_and_cleanup() { | |||
local command="$3" | |||
local cleanup_command="$4" | |||
|
|||
for (( i = 0; i < ${retriesNumber}; i++ )) | |||
for i in $(seq 1 ${retriesNumber}) |
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 is this better than the previous for loop version?
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.
This block of loop will only executes 1 time in this scenario. I found that there is also a for loop in start_confluent_schema_registry function:
for i in {1..30}; do
if get_and_verify_schema_subjects_exist; then
echo "Schema registry is up."
return 0
fi
echo "Waiting for schema registry..."
sleep 1
done
The variable i is set to 30 after this function, which cause the loop end after only 1 execute.
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 tried to reproduce the problem via
for i in {1..30}; do
echo $i
done
for (( i = 0; i < 10; i++ )) do
echo $i
done
When I execute this, then 1..30 and 0..9 will be printed. Hence, I'm not sure I really understand the problem you are describing. Could you provide me an example script to reproduce the problem with the old for loop?
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 think current scenario equals to the following script:
for (( i = 0; i < 10; i++ )) do
for i in {1..30}; do
echo $i
done
echo $i
done
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.
Ah this makes sense. Thanks a lot for your explanation.
Thanks for the review @tillrohrmann . I've updated the PR. Travis link https://travis-ci.org/KarmaGYZ/flink/builds/633584290 |
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.
Thanks for looking into my comments @KarmaGYZ. I fear I still don't understand the problem with the loops. I tried to reproduce what you've described but I wasn't able to do so. Could you provide me with a minimal example to reproduce the described problem?
@@ -748,7 +748,7 @@ function retry_times_with_backoff_and_cleanup() { | |||
local command="$3" | |||
local cleanup_command="$4" | |||
|
|||
for (( i = 0; i < ${retriesNumber}; i++ )) | |||
for i in $(seq 1 ${retriesNumber}) |
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 tried to reproduce the problem via
for i in {1..30}; do
echo $i
done
for (( i = 0; i < 10; i++ )) do
echo $i
done
When I execute this, then 1..30 and 0..9 will be printed. Hence, I'm not sure I really understand the problem you are describing. Could you provide me an example script to reproduce the problem with the old for loop?
Thanks for the review @tillrohrmann . I've removed the |
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.
Thanks a lot for the hardening and the explanation why it works @KarmaGYZ. LGTM. Merging this PR now.
@@ -748,7 +748,7 @@ function retry_times_with_backoff_and_cleanup() { | |||
local command="$3" | |||
local cleanup_command="$4" | |||
|
|||
for (( i = 0; i < ${retriesNumber}; i++ )) | |||
for i in $(seq 1 ${retriesNumber}) |
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.
Ah this makes sense. Thanks a lot for your explanation.
What is the purpose of the change
Harden the Avro Confluent Schema Registry nightly end-to-end test.
Brief change log
retry_times_with_backoff_and_cleanup
function.retry_times_with_backoff_and_cleanup
Verifying this change
Trigger the Avro Confluent Schema Registry nightly end-to-end test.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation