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: Remove reading of the data files to fix flakiness #9451

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

pvary
Copy link
Contributor

@pvary pvary commented Jan 9, 2024

No description provided.

@github-actions github-actions bot added the flink label Jan 9, 2024
@pvary pvary requested a review from stevenzwu January 9, 2024 17:28
@pvary
Copy link
Contributor Author

pvary commented Jan 9, 2024

@stevenzwu: I can not reproduce the issue locally. The actual reading of the data is not part of the Flink side test for WA. The tests are just checking the metrics there. Probably it would be enough on our side as well. Let's try out if this is still flaky, or not.

Copy link
Contributor

@stevenzwu stevenzwu left a comment

Choose a reason for hiding this comment

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

I assume this is a different kind of flakiness than the class loader check.

@manuzhang
Copy link
Contributor

I doubt we will still have flakiness as long as the following exists

  CommonTestUtils.waitForAllTaskRunning(
miniClusterResource.getMiniCluster(), jobClient.getJobID(), false);

@pvary
Copy link
Contributor Author

pvary commented Jan 10, 2024

I doubt we will still have flakiness as long as the following exists

  CommonTestUtils.waitForAllTaskRunning(
miniClusterResource.getMiniCluster(), jobClient.getJobID(), false);

I have seen the classloader issue in a CI run yesterday. That code contained the CommonTestUtils.waitForAllTaskRunning line.

I am really confused, because I couldn't reproduce the issue in any way locally, but seen it happen quite frequently on CI. This change is mostly a stab in the dark, but removes the code lines which make the issue to surface.

@ajantha-bhat
Copy link
Member

I have seen the classloader issue in a CI run yesterday

Ack. I have seen it too after disabling that class loader check PR and reported it here
#9408 (comment)

@stevenzwu stevenzwu merged commit 13d2160 into apache:main Jan 16, 2024
13 checks passed
@stevenzwu
Copy link
Contributor

@pvary thx for the explanation. let's give it a try then. if it still doesn't fix it, let's disable/ignore this test for now.

@pvary
Copy link
Contributor Author

pvary commented Jan 16, 2024

@stevenzwu: Ahh.. I forgot to merge. Thanks for finding and merging this!

@pvary pvary deleted the fix_flaky branch January 16, 2024 17:25
geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024
Co-authored-by: Peter Vary <peter_vary4@apple.com>
adnanhemani pushed a commit to adnanhemani/iceberg that referenced this pull request Jan 30, 2024
Co-authored-by: Peter Vary <peter_vary4@apple.com>
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
Co-authored-by: Peter Vary <peter_vary4@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants