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

[BEAM-8446] Adding a test checking the wait for BQ jobs #9858

Merged
merged 6 commits into from Oct 25, 2019

Conversation

pabloem
Copy link
Member

@pabloem pabloem commented Oct 23, 2019

Adding a test for the logic addedin PR #9856.

r: @ttanay
cc: @Ardagan


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).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • 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.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
--- Build Status
Build Status
Build Status
Build Status
--- --- Build Status
XLang --- --- --- Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status
Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@ttanay
Copy link
Contributor

ttanay commented Oct 24, 2019

The tests seem to be failing because of some unrelated reason.

@ttanay
Copy link
Contributor

ttanay commented Oct 24, 2019

Run Python PreCommit

Copy link
Contributor

@Ardagan Ardagan left a comment

Choose a reason for hiding this comment

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

LGTM overall. Please verify tests pass before merge and get someone else to approve since I don't have much experience with IOs.

@pabloem
Copy link
Member Author

pabloem commented Oct 24, 2019

Run Python PreCommit

@@ -424,6 +424,87 @@ def test_records_traverse_transform_with_mocks(self):
assert_that(jobs,
equal_to([job_reference]), label='CheckJobs')

@mock.patch('time.sleep')
def test_wait_for_job_completion(self, sleep_mock):
logging.getLogger().setLevel(logging.INFO)
Copy link
Contributor

Choose a reason for hiding this comment

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

Reducing log level to INFO outputs unneccesary stuff like:

INFO:root:==================== <function fix_side_input_pcoll_coders at 0x7fbbfb2d3730> ====================
INFO:root:==================== <function lift_combiners at 0x7fbbfb2d37b8> ====================
INFO:root:==================== <function expand_sdf at 0x7fbbfb2d3840> ====================
INFO:root:==================== <function expand_gbk at 0x7fbbfb2d38c8> ====================
INFO:root:==================== <function sink_flattens at 0x7fbbfb2d39d8> ====================
INFO:root:==================== <function greedily_fuse at 0x7fbbfb2d3a60> ====================
INFO:root:==================== <function read_to_impulse at 0x7fbbfb2d3ae8> ====================
INFO:root:==================== <function impulse_to_input at 0x7fbbfb2d3b70> ====================
INFO:root:==================== <function inject_timer_pcollections at 0x7fbbfb2d3d08> ====================
INFO:root:==================== <function sort_stages at 0x7fbbfb2d3d90> ====================
INFO:root:==================== <function window_pcollection_coders at 0x7fbbfb2d3e18> ====================
INFO:root:Creating state cache with size 100
INFO:root:Created Worker handler <apache_beam.runners.portability.fn_api_runner.EmbeddedWorkerHandler object at 0x7fbbfb2927b8> for environment urn: "beam:env:embedded_python:v1"

INFO:root:Running (((ref_AppliedPTransform_assert_that/Create/Read_9)+(ref_AppliedPTransform_assert_that/Group/pair_with_0_13))+(assert_that/Group/Flatten/Transcode/0))+(assert_that/Group/Flatten/Write/0)
INFO:root:Running (ref_AppliedPTransform_job_ref/Read_3)+(ref_PCollection_PCollection_1/Write)
INFO:root:Running ((((((ref_AppliedPTransform_Create/Read_5)+(ref_AppliedPTransform_ParDo(WaitForBQJobs)_6))+(ref_AppliedPTransform_assert_that/WindowInto(WindowIntoFn)_10))+(ref_AppliedPTransform_assert_that/ToVoidKey_11))+(ref_AppliedPTransform_assert_that/Group/pair_with_1_14))+(assert_that/Group/Flatten/Transcode/1))+(assert_that/Group/Flatten/Write/1)
INFO:root:Job status: <Mock name='mock.status' id='140445338423704'>
INFO:root:Job status: <Mock name='mock.status' id='140445338424376'>
INFO:root:Job status: <Mock name='mock.status' id='140445338424992'>
INFO:root:Running (assert_that/Group/Flatten/Read)+(assert_that/Group/GroupByKey/Write)
INFO:root:Running (((assert_that/Group/GroupByKey/Read)+(ref_AppliedPTransform_assert_that/Group/Map(_merge_tagged_vals_under_key)_20))+(ref_AppliedPTransform_assert_that/Unkey_21))+(ref_AppliedPTransform_assert_that/Match_22)

Can this be removed if it's not being used in the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

great catch. thanks tanay!

Copy link
Contributor

Choose a reason for hiding this comment

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

:)

job_2_done.status.state = 'DONE'
job_2_done.status.errorResult = None

job_1_done = mock.Mock()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Is job_1_error or something more appropriate, since the job errored out, but the status is DONE?
Can be confused with job_2_done which actually succeeds.

@pabloem
Copy link
Member Author

pabloem commented Oct 25, 2019

Run Python PreCommit

@pabloem
Copy link
Member Author

pabloem commented Oct 25, 2019

Run Portable_Python PreCommit

@pabloem
Copy link
Member Author

pabloem commented Oct 25, 2019

Run Python PreCommit

@ttanay
Copy link
Contributor

ttanay commented Oct 25, 2019

LGTM! Thanks Pablo!

@pabloem
Copy link
Member Author

pabloem commented Oct 25, 2019

the new tests seem to be failing in py 2.7 - I suspect due to pickling issues. I think I'll skip them on py2.7

job_2_done.status.errorResult = None

job_1_error = mock.Mock()
job_1_error.status.state = 'DONE'
Copy link
Contributor

Choose a reason for hiding this comment

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

Does error still returns 'DONE' as state?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it returns DONE with an error report

@pabloem pabloem merged commit 4b5f7e5 into apache:master Oct 25, 2019
@pabloem pabloem deleted the bqwait-test branch October 25, 2019 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants