🐛 [On call / 171] Source Salesforce: fixed the bug when `Bulk` fetch took all memory of kube pods #11692

bazarnov · 2022-04-04T11:42:18Z

What

Resolving: https://github.com/airbytehq/oncall/issues/171

How

refactored download_data method to write the response by chunks into tmp file
added read_with_chunks method to read the data from tmp file by chunks (lines)
added test_memory unit_test to cover the case
edited Dockerfile to support native installation of pandas and numpy frameworks

🚨 User Impact 🚨

No impact expected.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the new connector version is published, connector version bumped in the seed directory as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

codecov · 2022-04-04T12:08:29Z

Codecov Report

❗ No coverage uploaded for pull request base (master@1f6eff7). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 6beb666 differs from pull request most recent head 8ce95f5. Consider uploading reports for the commit 8ce95f5 to get more accurate results

@@            Coverage Diff            @@
##             master   #11692   +/-   ##
=========================================
  Coverage          ?   88.11%           
=========================================
  Files             ?        7           
  Lines             ?      547           
  Branches          ?        0           
=========================================
  Hits              ?      482           
  Misses            ?       65           
  Partials          ?        0

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 1f6eff7...8ce95f5. Read the comment docs.

…salesforce-oom-issue

bazarnov · 2022-04-04T14:16:00Z

/test connector=connectors/source-salesforce

🕑 connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/2090450627
✅ connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/2090450627
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        74      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/utils/common.py                  70     17    76%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/tests/test_core.py              275    106    61%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
source_acceptance_test/tests/test_incremental.py        69     38    45%
------------------------------------------------------------------------
TOTAL                                                  876    259    70%
Name                                 Stmts   Miss  Cover
--------------------------------------------------------
source_salesforce/exceptions.py          2      0   100%
source_salesforce/__init__.py            2      0   100%
source_salesforce/api.py               150     19    87%
source_salesforce/streams.py           262     54    79%
source_salesforce/rate_limiting.py      22      6    73%
source_salesforce/source.py             75     33    56%
source_salesforce/utils.py               8      7    12%
--------------------------------------------------------
TOTAL                                  521    119    77%
Name                                 Stmts   Miss  Cover
--------------------------------------------------------
source_salesforce/utils.py               8      0   100%
source_salesforce/exceptions.py          2      0   100%
source_salesforce/__init__.py            2      0   100%
source_salesforce/source.py             75      6    92%
source_salesforce/api.py               150     14    91%
source_salesforce/rate_limiting.py      22      3    86%
source_salesforce/streams.py           262     39    85%
--------------------------------------------------------
TOTAL                                  521     62    88%

grubberr · 2022-04-05T20:16:00Z

I have added unit_tests/test_memory.py file PLEASE REMOVE !!!

in really I still cannot see any difference

git checkout bazrnov/171-source-salesforce-oom-issue
pytest -s unit_tests/test_memory.py
first_size = 0.10 Mb, first_peak = 16.71 Mb

git checkout master
pytest -s unit_tests/test_memory.py
first_size = 0.10 Mb, first_peak = 16.71 Mb

Signed-off-by: Sergey Chvalyuk <[email protected]>

bazarnov · 2022-04-05T20:37:18Z

Can we adjust this test to use all memory in the system available? Let’s say 10gb file?

…

________________________________ From: Serhii Chvaliuk ***@***.***> Sent: Tuesday, April 5, 2022 11:16:11 PM To: airbytehq/airbyte ***@***.***> Cc: Baz ***@***.***>; Assign ***@***.***> Subject: Re: [airbytehq/airbyte] 🐛 [On call / 171] Source Salesforce: fixed the bug when `Bulk` fetch took all memory of kube pods (PR #11692) I have added unit_tests/test_memory.py file PLEASE REMOVE !!! in really I still cannot see any difference git checkout bazrnov/171-source-salesforce-oom-issue pytest -s unit_tests/test_memory.py first_size = 0.10 Mb, first_peak = 16.71 Mb git checkout master pytest -s unit_tests/test_memory.py first_size = 0.10 Mb, first_peak = 16.71 Mb — Reply to this email directly, view it on GitHub<#11692 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AFPMHGRF7R3I2XXWH7NG24DVDSNQXANCNFSM5SPH4HPA>. You are receiving this because you were assigned.Message ID: ***@***.***>

…salesforce-oom-issue

bazarnov · 2022-04-12T23:00:54Z

/test connector=connectors/source-salesforce

🕑 connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/2158014423
✅ connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/2158014423
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        74      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/utils/common.py                  70     17    76%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/tests/test_core.py              285    106    63%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
source_acceptance_test/tests/test_incremental.py        69     38    45%
------------------------------------------------------------------------
TOTAL                                                  886    259    71%
Name                                 Stmts   Miss  Cover
--------------------------------------------------------
source_salesforce/__init__.py            2      0   100%
source_salesforce/exceptions.py          8      1    88%
source_salesforce/api.py               150     19    87%
source_salesforce/streams.py           282     60    79%
source_salesforce/rate_limiting.py      22      6    73%
source_salesforce/source.py             75     33    56%
source_salesforce/utils.py               8      7    12%
--------------------------------------------------------
TOTAL                                  547    126    77%
Name                                 Stmts   Miss  Cover
--------------------------------------------------------
source_salesforce/utils.py               8      0   100%
source_salesforce/__init__.py            2      0   100%
source_salesforce/source.py             75      6    92%
source_salesforce/api.py               150     14    91%
source_salesforce/exceptions.py          8      1    88%
source_salesforce/rate_limiting.py      22      3    86%
source_salesforce/streams.py           282     41    85%
--------------------------------------------------------
TOTAL                                  547     65    88%

girarda

small question around the TmpFileIOError's constructor, but looks good otherwise

airbyte-integrations/connectors/source-salesforce/source_salesforce/exceptions.py

bazarnov · 2022-04-13T17:12:58Z

/publish connector=connectors/source-salesforce

🕑 connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/2162692116
🚀 Successfully published connectors/source-salesforce
🚀 Auto-bumped version for connectors/source-salesforce
✅ connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/2162692116

…took all memory of kube pods (#11692)

fixed

29833d8

bazarnov self-assigned this Apr 4, 2022

github-actions bot added the area/connectors Connector related issues label Apr 4, 2022

updated changelog

e74f121

github-actions bot added the area/documentation Improvements or additions to documentation label Apr 4, 2022

bazarnov temporarily deployed to more-secrets April 4, 2022 11:45 Inactive

bazarnov changed the title ~~🐛 [On call / 171] Source Salesforce: fixed the bug when Bulk fetch took all memory of kube pods.~~ 🐛 [On call / 171] Source Salesforce: fixed the bug when Bulk fetch took all memory of kube pods Apr 4, 2022

bazarnov requested review from grubberr and annalvova05 April 4, 2022 12:03

bazarnov requested a review from midavadim April 4, 2022 13:39

bazarnov added 2 commits April 4, 2022 17:13

Merge remote-tracking branch 'origin/master' into bazrnov/171-source-…

b103fd0

…salesforce-oom-issue

fixed integration tests

fac212f

bazarnov temporarily deployed to more-secrets April 4, 2022 14:17 Inactive

bazarnov temporarily deployed to more-secrets April 4, 2022 14:18 Inactive

grubberr temporarily deployed to more-secrets April 5, 2022 20:07 Inactive

midavadim approved these changes Apr 5, 2022

View reviewed changes

grubberr temporarily deployed to more-secrets April 5, 2022 20:16 Inactive

github-actions bot referenced this pull request Apr 5, 2022

fix

2885769

Signed-off-by: Sergey Chvalyuk <[email protected]>

bazarnov added 3 commits April 7, 2022 15:15

Merge remote-tracking branch 'origin/master' into bazrnov/171-source-…

e732a8c

…salesforce-oom-issue

refactored the fix

d0635fa

Merge remote-tracking branch 'origin/master' into bazrnov/171-source-…

804b5f1

…salesforce-oom-issue

bazarnov force-pushed the bazrnov/171-source-salesforce-oom-issue branch from 2885769 to 804b5f1 Compare April 8, 2022 23:22

bazarnov temporarily deployed to more-secrets April 8, 2022 23:24 Inactive

bazarnov added 2 commits April 12, 2022 23:23

fixed the bulk_error_test

8416099

updated after review

f85b81f

bazarnov temporarily deployed to more-secrets April 12, 2022 22:45 Inactive

fixed flakecheck

e825538

bazarnov temporarily deployed to more-secrets April 12, 2022 23:02 Inactive

bazarnov requested a review from girarda April 13, 2022 09:15

girarda approved these changes Apr 13, 2022

View reviewed changes

airbyte-integrations/connectors/source-salesforce/source_salesforce/exceptions.py Outdated Show resolved Hide resolved

updated after review

7cb95f3

bazarnov temporarily deployed to more-secrets April 13, 2022 16:59 Inactive

fixed tests

6beb666

bazarnov temporarily deployed to more-secrets April 13, 2022 17:14 Inactive

new specs

8ce95f5

octavia-squidington-iii temporarily deployed to more-secrets April 13, 2022 17:32 Inactive

bazarnov force-pushed the bazrnov/171-source-salesforce-oom-issue branch from cb15d1c to 8ce95f5 Compare April 13, 2022 17:33

bazarnov merged commit 644ace4 into master Apr 13, 2022

bazarnov deleted the bazrnov/171-source-salesforce-oom-issue branch April 13, 2022 17:35

bazarnov temporarily deployed to more-secrets April 13, 2022 17:35 Inactive

This was referenced Apr 14, 2022

Bump Airbyte version from 0.35.67-alpha to 0.35.68-alpha #11992

Closed

Bump Airbyte version from 0.35.67-alpha to 0.35.68-alpha #12015

Merged

suhomud pushed a commit that referenced this pull request May 23, 2022

🐛 [On call / 171] Source Salesforce: fixed the bug when Bulk fetch …

eeddaac

…took all memory of kube pods (#11692)

Nakachi-S mentioned this pull request Sep 14, 2022

🐛 Source Salesforce: Write binary file without decoding #16684

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 [On call / 171] Source Salesforce: fixed the bug when `Bulk` fetch took all memory of kube pods #11692

🐛 [On call / 171] Source Salesforce: fixed the bug when `Bulk` fetch took all memory of kube pods #11692

bazarnov commented Apr 4, 2022 •

edited

Loading

codecov bot commented Apr 4, 2022 •

edited

Loading

bazarnov commented Apr 4, 2022 •

edited by github-actions bot

Loading

grubberr commented Apr 5, 2022

bazarnov commented Apr 5, 2022 via email

bazarnov commented Apr 12, 2022 •

edited by github-actions bot

Loading

girarda left a comment

bazarnov commented Apr 13, 2022 •

edited by github-actions bot

Loading

🐛 [On call / 171] Source Salesforce: fixed the bug when Bulk fetch took all memory of kube pods #11692

🐛 [On call / 171] Source Salesforce: fixed the bug when Bulk fetch took all memory of kube pods #11692

Conversation

bazarnov commented Apr 4, 2022 • edited Loading

What

How

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

codecov bot commented Apr 4, 2022 • edited Loading

Codecov Report

bazarnov commented Apr 4, 2022 • edited by github-actions bot Loading

grubberr commented Apr 5, 2022

bazarnov commented Apr 5, 2022 via email

bazarnov commented Apr 12, 2022 • edited by github-actions bot Loading

girarda left a comment

Choose a reason for hiding this comment

bazarnov commented Apr 13, 2022 • edited by github-actions bot Loading

🐛 [On call / 171] Source Salesforce: fixed the bug when `Bulk` fetch took all memory of kube pods #11692

🐛 [On call / 171] Source Salesforce: fixed the bug when `Bulk` fetch took all memory of kube pods #11692

bazarnov commented Apr 4, 2022 •

edited

Loading

codecov bot commented Apr 4, 2022 •

edited

Loading

bazarnov commented Apr 4, 2022 •

edited by github-actions bot

Loading

bazarnov commented Apr 12, 2022 •

edited by github-actions bot

Loading

bazarnov commented Apr 13, 2022 •

edited by github-actions bot

Loading