Skip to content

Conversation

@rmetzger
Copy link
Contributor

@rmetzger rmetzger commented Mar 4, 2020

What is the purpose of the change

The docker tests are not passing on JDK11. Until this is implemented, we need to skip them

Brief change log

  • Skip tests using docker somehow in the run-nightly-tests.sh script.

Verifying this change

Set the jdk: jdk11 in azure-pipelines.yml and see the e2e tests passing

@flinkbot
Copy link
Collaborator

flinkbot commented Mar 4, 2020

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 51918c2 (Wed Mar 04 12:44:10 UTC 2020)

Warnings:

  • No documentation files were touched! Remember to keep the Flink docs up to date!

Mention the bot in a comment to re-run the automated checks.

Review Progress

  • ❓ 1. The [description] looks good.
  • ❓ 2. There is [consensus] that the contribution should go into to Flink.
  • ❓ 3. Needs [attention] from.
  • ❓ 4. The change fits into the overall [architecture].
  • ❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

Details
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 commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier

@flinkbot
Copy link
Collaborator

flinkbot commented Mar 4, 2020

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run travis re-run the last Travis build
  • @flinkbot run azure re-run the last Azure build


run_test "Heavy deployment end-to-end test" "$END_TO_END_DIR/test-scripts/test_heavy_deployment.sh" "skip_check_exceptions"

run_test "ConnectedComponents iterations with high parallelism end-to-end test" "$END_TO_END_DIR/test-scripts/test_high_parallelism_iterations.sh 25"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would refrain from excluding this test until we have a confirmation that this is indeed java 11 related and some numbers of how reliably it fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can not remember having seen this failure on jdk8 ... In the last 48 hours, I have changed my policy to creating tickets for each test failure I see.

How many successful executions on master is proof enough that the test is stable on jdk8? (I propose 10)
How long should I keep the JIRA ticket open until we believe it is a jdk11 only issue? (I propose a week)

If we are not excluding this test for the JDK11 build, we won't have e2e coverage for jdk11 for everything that comes after.

Copy link
Contributor

Choose a reason for hiding this comment

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

This test has always been incredibly flaky, that's why I advise against rash calls.

Trivial solution for preventing other tests from running in case of failures is to move it to the end of the script.

Copy link
Contributor

Choose a reason for hiding this comment

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

naturally if you can show that it is reproducible on java 11 then we can of course disable it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I am able to fix the test on JDK11: https://issues.apache.org/jira/browse/FLINK-16417

# - script: FLINK_DIR=build-target ./flink-end-to-end-tests/run-pre-commit-tests.sh
# displayName: Test - precommit
- script: FLINK_DIR=`pwd`/build-target flink-end-to-end-tests/run-nightly-tests.sh
- script: ${{parameters.environment}} FLINK_DIR=`pwd`/build-target flink-end-to-end-tests/run-nightly-tests.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

should be a separate commit, and possibly even JIRA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

What this does is passing the PROFILE variable to the script. This is necessary for deciding whether we are in a jdk11 setting or not. ... I see a clear relationship to the title of the JIRA I'm working on.

Copy link
Contributor

Choose a reason for hiding this comment

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

because the script was already relying on this being set for the avro schema registry. This fixes a bug, it's not an improvement.

@rmetzger
Copy link
Contributor Author

rmetzger commented Mar 5, 2020

I addressed your comments:

  • ConnectedComponents is executed in all cases
  • passing the PROFILE is in a separate JIRA

Copy link
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

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

+1

@rmetzger rmetzger merged commit 1a4fb25 into apache:master Mar 5, 2020
@rmetzger
Copy link
Contributor Author

rmetzger commented Mar 5, 2020

Thank you for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants