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

#20812 handle @RequiresStableInput in portable flink #22889

Merged

Conversation

je-ik
Copy link
Contributor

@je-ik je-ik commented Aug 25, 2022

Fixes #20812
Fixes #21333

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI.

@je-ik je-ik force-pushed the issue-20812-requires-stable-input-portable branch 2 times, most recently from 0b076df to 54543bb Compare August 25, 2022 13:01
@je-ik
Copy link
Contributor Author

je-ik commented Aug 25, 2022

R: @ibzib @dmvk

@je-ik je-ik requested review from ibzib and dmvk August 25, 2022 13:01
@je-ik je-ik force-pushed the issue-20812-requires-stable-input-portable branch 3 times, most recently from d33b6f1 to 7f54f69 Compare August 25, 2022 13:38
@github-actions
Copy link
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@github-actions
Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

@je-ik je-ik force-pushed the issue-20812-requires-stable-input-portable branch from 7f54f69 to f7d3e5d Compare August 29, 2022 07:40
@je-ik
Copy link
Contributor Author

je-ik commented Aug 29, 2022

assign set of reviewers

@je-ik
Copy link
Contributor Author

je-ik commented Aug 30, 2022

Run Java Flink PortableValidatesRunner Streaming

@je-ik je-ik changed the title #20812 handle @RequiresStableInput in portable flink [DO NOT MERGE] #20812 handle @RequiresStableInput in portable flink Aug 30, 2022
@je-ik je-ik force-pushed the issue-20812-requires-stable-input-portable branch from f7d3e5d to 3e97222 Compare September 5, 2022 09:35
@je-ik je-ik changed the title [DO NOT MERGE] #20812 handle @RequiresStableInput in portable flink #20812 handle @RequiresStableInput in portable flink Sep 5, 2022
@je-ik
Copy link
Contributor Author

je-ik commented Sep 5, 2022

Run Flink ValidatesRunner

@je-ik
Copy link
Contributor Author

je-ik commented Sep 5, 2022

Run Java Flink PortableValidatesRunner Streaming

@je-ik je-ik force-pushed the issue-20812-requires-stable-input-portable branch from 3e97222 to 9445883 Compare September 5, 2022 11:34
@je-ik
Copy link
Contributor Author

je-ik commented Sep 5, 2022

Run Flink ValidatesRunner

@je-ik
Copy link
Contributor Author

je-ik commented Sep 5, 2022

Run Java Flink PortableValidatesRunner Streaming

@je-ik
Copy link
Contributor Author

je-ik commented Sep 5, 2022

Run Flink ValidatesRunner

@je-ik
Copy link
Contributor Author

je-ik commented Sep 5, 2022

Run Java Flink PortableValidatesRunner Streaming

@je-ik je-ik requested a review from lukecwik September 5, 2022 14:19
@je-ik
Copy link
Contributor Author

je-ik commented Sep 5, 2022

R: @lukecwik

@je-ik
Copy link
Contributor Author

je-ik commented Sep 27, 2022

@aaltay @robertwb @kennknowles Looks I'm unable to find a reviewer for this one. It fixes correctness bug in Flink, cau you please help find one? Thanks. 👍

@aaltay aaltay requested a review from kileys September 27, 2022 16:02
@aaltay
Copy link
Member

aaltay commented Sep 27, 2022

@kileys - Would you be able to review this? It is a medium sized change and does not require too much flink runner context.

@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #22889 (0694831) into master (0f423dd) will increase coverage by 7.98%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #22889      +/-   ##
==========================================
+ Coverage   65.20%   73.19%   +7.98%     
==========================================
  Files         731      734       +3     
  Lines       97960    98897     +937     
==========================================
+ Hits        63878    72390    +8512     
+ Misses      32727    25152    -7575     
  Partials     1355     1355              
Flag Coverage Δ
python 82.81% <ø> (+11.44%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...apache_beam/runners/portability/portable_runner.py 75.60% <0.00%> (-1.72%) ⬇️
...hon/apache_beam/runners/worker/bundle_processor.py 93.54% <0.00%> (-0.25%) ⬇️
...am/testing/benchmarks/cloudml/criteo_tft/criteo.py 0.00% <0.00%> (ø)
...chmarks/cloudml/cloudml_benchmark_constants_lib.py 100.00% <0.00%> (ø)
...m/testing/benchmarks/cloudml/pipelines/workflow.py 6.94% <0.00%> (ø)
...ks/python/apache_beam/runners/worker/operations.py 74.32% <0.00%> (+0.14%) ⬆️
sdks/python/apache_beam/transforms/environments.py 88.26% <0.00%> (+0.29%) ⬆️
...eam/runners/interactive/interactive_environment.py 92.02% <0.00%> (+0.30%) ⬆️
...ks/python/apache_beam/runners/worker/sdk_worker.py 89.24% <0.00%> (+0.48%) ⬆️
...python/apache_beam/runners/worker/worker_status.py 75.33% <0.00%> (+0.66%) ⬆️
... and 160 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@je-ik je-ik force-pushed the issue-20812-requires-stable-input-portable branch 3 times, most recently from 842a84f to d597e4e Compare December 22, 2022 15:14
@je-ik
Copy link
Contributor Author

je-ik commented Dec 22, 2022

@lukecwik is there any way to get logs of the failed pre commit?
https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/25421/

The tests pass locally, so without the log I'm not sure how to fix the problem. :-/

@je-ik je-ik force-pushed the issue-20812-requires-stable-input-portable branch from d597e4e to 1d14adf Compare December 23, 2022 10:06
@je-ik je-ik force-pushed the issue-20812-requires-stable-input-portable branch 2 times, most recently from 80389b4 to 7e12621 Compare January 2, 2023 17:13
@je-ik je-ik force-pushed the issue-20812-requires-stable-input-portable branch from 7e12621 to 5919a8d Compare January 2, 2023 17:23
@je-ik je-ik force-pushed the issue-20812-requires-stable-input-portable branch from 5919a8d to dbd0997 Compare January 3, 2023 09:11
@je-ik je-ik force-pushed the issue-20812-requires-stable-input-portable branch from dbd0997 to bb622e4 Compare January 3, 2023 12:24
@je-ik je-ik changed the title [DO NOT MERGE] #20812 handle @RequiresStableInput in portable flink #20812 handle @RequiresStableInput in portable flink Jan 3, 2023
@je-ik
Copy link
Contributor Author

je-ik commented Jan 3, 2023

@lukecwik I finally managed to make the tests pass, so this is ready for final review. I will squash the commits after.

@github-actions github-actions bot removed the website label Jan 3, 2023
@je-ik je-ik requested a review from lukecwik January 3, 2023 16:54
@je-ik
Copy link
Contributor Author

je-ik commented Jan 23, 2023

@lukecwik would you have time for review, or could you recommend someone please?

Copy link
Member

@lukecwik lukecwik left a comment

Choose a reason for hiding this comment

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

LGTM

@lukecwik
Copy link
Member

I'm glad that you were able to work through the problems and get the tests fixed and passing.

@lukecwik lukecwik merged commit b1aba9d into apache:master Jan 23, 2023
@je-ik je-ik deleted the issue-20812-requires-stable-input-portable branch January 24, 2023 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants