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-14422] Adding exception testing for ReadFromBigQuery and WriteToBigQuery #17589

Merged
merged 21 commits into from
Jun 10, 2022

Conversation

ahmedabu98
Copy link
Contributor

No description provided.

@asf-ci
Copy link

asf-ci commented May 9, 2022

Can one of the admins verify this patch?

4 similar comments
@asf-ci
Copy link

asf-ci commented May 9, 2022

Can one of the admins verify this patch?

@asf-ci
Copy link

asf-ci commented May 9, 2022

Can one of the admins verify this patch?

@asf-ci
Copy link

asf-ci commented May 9, 2022

Can one of the admins verify this patch?

@asf-ci
Copy link

asf-ci commented May 9, 2022

Can one of the admins verify this patch?

@ahmedabu98 ahmedabu98 changed the title Exception testing for ReadFromBigQuery [BEAM-14422] Exception testing for ReadFromBigQuery May 10, 2022
query='SELECT * FROM `project.dataset.table`',
gcs_location='gs://temp_location')

self.assertEqual(16, mock_api.call_count)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We expect 16 calls here because get_or_create_dataset (where the dataset Insert API is called from) has the @retry... decorator and create_temporary_dataset, where get_or_create_dataset is called from, also has the @retry... decorator.

For each create_temporary_dataset retry, we have four get_or_create_dataset retries, giving us a total of 16.
Having 16 retries seems weird, but @pabloem and I thought this might be by design to emulate an exponential backoff and may be of benefit to users. Thoughts on this?

@johnjcasey
@chamikaramj
@ihji

@ahmedabu98
Copy link
Contributor Author

R: @pabloem

@pabloem
Copy link
Member

pabloem commented May 12, 2022

Run Portable_Python PreCommit

1 similar comment
@pabloem
Copy link
Member

pabloem commented May 12, 2022

Run Portable_Python PreCommit

@pabloem
Copy link
Member

pabloem commented May 12, 2022

trying a fix myself : )

exception_type=exceptions.ServiceUnavailable,
error_message='backendError'),
])
@mock.patch('time.sleep')
Copy link
Member

Choose a reason for hiding this comment

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

also try moving this into a context manager?

@pabloem
Copy link
Member

pabloem commented Jun 1, 2022

Run Python PreCommit

@pabloem
Copy link
Member

pabloem commented Jun 8, 2022

@ahmedabu98 I gave up on figuring out the env difference and change the assertion to just check assert_called

@pabloem
Copy link
Member

pabloem commented Jun 8, 2022

Run Python 3.8 PostCommit

@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #17589 (d21dd3b) into master (a6ea148) will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #17589      +/-   ##
==========================================
+ Coverage   73.86%   73.95%   +0.08%     
==========================================
  Files         690      695       +5     
  Lines       91211    91883     +672     
==========================================
+ Hits        67374    67948     +574     
- Misses      22605    22703      +98     
  Partials     1232     1232              
Flag Coverage Δ
python 83.71% <ø> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...ache_beam/runners/dataflow/dataflow_job_service.py 50.00% <0.00%> (-12.17%) ⬇️
...s/interactive/dataproc/dataproc_cluster_manager.py 77.41% <0.00%> (-10.49%) ⬇️
...ache_beam/runners/dataflow/test_dataflow_runner.py 29.62% <0.00%> (-3.03%) ⬇️
sdks/python/apache_beam/coders/row_coder.py 94.49% <0.00%> (-2.65%) ⬇️
...eam/runners/portability/fn_api_runner/fn_runner.py 87.51% <0.00%> (-2.50%) ⬇️
sdks/python/apache_beam/utils/retry.py 83.05% <0.00%> (-2.39%) ⬇️
sdks/python/apache_beam/internal/gcp/auth.py 78.66% <0.00%> (-2.37%) ⬇️
sdks/python/apache_beam/runners/common.py 87.94% <0.00%> (-2.33%) ⬇️
...nners/portability/fn_api_runner/worker_handlers.py 77.89% <0.00%> (-1.45%) ⬇️
sdks/python/apache_beam/internal/dill_pickler.py 84.89% <0.00%> (-1.44%) ⬇️
... and 65 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6ea148...d21dd3b. Read the comment docs.

@pabloem
Copy link
Member

pabloem commented Jun 8, 2022

Run Python PreCommit

@ahmedabu98
Copy link
Contributor Author

Run Python 3.8 PostCommit

@ahmedabu98
Copy link
Contributor Author

@ahmedabu98 I gave up on figuring out the env difference and change the assertion to just check assert_called

I think assert_called is good enough haha. Thanks for working on it!

@pabloem
Copy link
Member

pabloem commented Jun 10, 2022

lgtm thanks!

@pabloem pabloem merged commit 0653395 into apache:master Jun 10, 2022
@ahmedabu98 ahmedabu98 deleted the bq_py_exception_handling branch June 10, 2022 18:50
@aaltay
Copy link
Member

aaltay commented Jun 16, 2022

Potentially release blocking for 2.40.0. (/cc @tvalentyn @pabloem )

bullet03 pushed a commit to akvelon/beam that referenced this pull request Jun 20, 2022
…or ReadFromBigQuery

* create temp dataset exception test

* working with real dataset and table, need to make it work with dummy

* error message is correct

* temp dataset exception done

* query exception test + some fixes

* export read job exception

* exception test for load job fail

* trying fix

* copy load job exception draft

* using context managers instead of decorators for patch in read exception tests

* using context manager for file loads exceptions

* test for copy load job exceptions

* ignore

* make create temp dataset exception faster

* reducing number of exceptions for copy load job

* ignore

* testing less exception types to reduce test speed

* fix formatter

* fix lint

* fixup

Co-authored-by: Pablo E <pabloem@apache.org>
Co-authored-by: Pablo <pabloem@users.noreply.github.com>
@ahmedabu98 ahmedabu98 changed the title [BEAM-14422] Exception testing for ReadFromBigQuery [BEAM-14422] Adding exception testing for ReadFromBigQuery and WriteToBigQuery Jul 27, 2022
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.

4 participants