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-32963][Test] Remove flakiness from testKeyedMapStateStateMigration #23298

Closed
wants to merge 1 commit into from

Conversation

asha-boyapati
Copy link
Contributor

@asha-boyapati asha-boyapati commented Aug 26, 2023

Update StateBackendMigrationTestBase.java2

What is the purpose of the change

This PR makes the following test stable: org.apache.flink.runtime.state.FileStateBackendMigrationTest.testKeyedMapStateStateMigration

The test is currently flaky because the order of elements returned by the iterator is non-deterministic.

This PR fixes the flaky test by making it independent of the order of elements returned by the iterator.

We detected this using the NonDex tool using the following command:

mvn edu.illinois:nondex-maven-plugin:2.1.1:nondex -pl flink-runtime -DnondexRuns=10 -Dtest=org.apache.flink.runtime.state.FileStateBackendMigrationTest#testKeyedMapStateStateMigration

Please see the following Continuous Integration log that shows the flakiness:
https://github.com/asha-boyapati/flink/actions/runs/5909136145/job/16029377793

Please see the following Continuous Integration log that shows that the flakiness is fixed by this change:
https://github.com/asha-boyapati/flink/actions/runs/5909183468/job/16029467973

The Jira ticket associated with this PR is as follows:
https://issues.apache.org/jira/browse/FLINK-32963

Brief change log

This PR fixes the flaky test by making it independent of the order of elements returned by the iterator.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

This change only modifies a test slightly and does not change any underlying code.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

@flinkbot
Copy link
Collaborator

flinkbot commented Aug 26, 2023

CI report:

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

Copy link
Contributor

@masteryhx masteryhx left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! PTAL my comments.

@asha-boyapati
Copy link
Contributor Author

Thank you for your comments. I made the changes according to your suggestions. I created a separate method called sortedIterator. And I used a TreeSet. Please take another look when you have time.

Copy link
Contributor

@masteryhx masteryhx left a comment

Choose a reason for hiding this comment

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

Thanks for the update.
LGTM % commit message, current message lack some infos, we could say "Fix xxx problem of xxx"
BTW, we could just mark the component tag as "state" or "test".
Could you squash all commits and rename the commit message ?

@asha-boyapati asha-boyapati changed the title [FLINK-32963][Runtime / State Backends] Update StateBackendMigrationTestBase.java [FLINK-32963][Test] Remove flakiness from testKeyedMapStateStateMigration Aug 28, 2023
@asha-boyapati
Copy link
Contributor Author

I squashed the commits and changed the commit message. I also changed the component tag to "Test" as you suggested. Please take another look.

Copy link
Contributor

@masteryhx masteryhx left a comment

Choose a reason for hiding this comment

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

LGTM

@asha-boyapati
Copy link
Contributor Author

I want to add that this PR not only removes flakiness from the test mentioned in the title and description (org.apache.flink.runtime.state.FileStateBackendMigrationTest.testKeyedMapStateStateMigration), but also from the following two tests:

org.apache.flink.runtime.state.FileStateBackendMigrationTest.testKeyedMapStateAsIs

and

org.apache.flink.runtime.state.FileStateBackendMigrationTest.testKeyedMapStateSerializerReconfiguration

All 3 tests call the function testKeyedMapStateUpgrade, which used to use unsorted iterators before this PR but uses sorted iterators after this PR. This PR thus removes the flakiness from all 3 tests in the same way.

I confirmed this by checking out the versions just before and after this commit and running the NonDex tool using the following commands:

mvn edu.illinois:nondex-maven-plugin:2.1.1:nondex -DnondexRuns=10 -pl flink-runtime -Dtest=org.apache.flink.runtime.state.FileStateBackendMigrationTest#testKeyedMapStateStateMigration

mvn edu.illinois:nondex-maven-plugin:2.1.1:nondex -DnondexRuns=10 -pl flink-runtime -Dtest=org.apache.flink.runtime.state.FileStateBackendMigrationTest#testKeyedMapStateAsIs

mvn edu.illinois:nondex-maven-plugin:2.1.1:nondex -DnondexRuns=10 -pl flink-runtime -Dtest=org.apache.flink.runtime.state.FileStateBackendMigrationTest#testKeyedMapStateSerializerReconfiguration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants