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

🐛 Source salesforce: processing of failed jobs #10141

Merged
merged 9 commits into from
Feb 10, 2022

Conversation

antixar
Copy link
Contributor

@antixar antixar commented Feb 7, 2022

What

The BULK SF logic can return "failed" statuses for different streams. As rule as this error message doesn't include any useful information e.g.:
Unexpected exception encountered in query processing. Please contact support with the following id: 326566388-63578 (-436445966). And this error can't be handled automatically.

How

The standard SF API (GET synchronous request) returns normal obvious error messages. Thus we can make additional request for loading of real failure reasons.

Enhancement:

  1. Minor refactoring: meta sobjects' options (different access/setting flags without sensitive data ) of each stream are available into each stream as the variable self.soject_options. This variable will be added to log messages if SF returns some unknown fail case. It will help to troubleshoot bugs in the future.
  2. Remove the spec parameter wait_timeout Source Salesforce: removal of the wait_timeout option #8104
  3. add a logic for switching to non-bulk method

🚨 User Impact 🚨

  1. all failed jobs will be skipped and user will be able to know about this into Airbyte logs only.
  2. the wait_timeout will vanish from connector's UI interfaces and after connector's upgrading the new logic will use default timeout value(10mins) only

Pre-merge Checklist

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • 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
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • 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
  • Credentials added to Github CI. 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

@antixar antixar self-assigned this Feb 7, 2022
@github-actions github-actions bot added the area/connectors Connector related issues label Feb 7, 2022
@antixar antixar linked an issue Feb 7, 2022 that may be closed by this pull request
@antixar
Copy link
Contributor Author

antixar commented Feb 7, 2022

/test connector=connectors/source-salesforce

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

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/config.py                        74      6    92%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/tests/test_core.py              275    106    61%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/tests/test_incremental.py        69     38    45%
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/utils/common.py                  70     17    76%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/utils/connector_runner.py       110     48    56%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
------------------------------------------------------------------------
TOTAL                                                  876    259    70%
Name                                 Stmts   Miss  Cover
--------------------------------------------------------
source_salesforce/__init__.py            2      0   100%
source_salesforce/api.py               126     33    74%
source_salesforce/exceptions.py          1      0   100%
source_salesforce/rate_limiting.py      22      6    73%
source_salesforce/source.py             74     34    54%
source_salesforce/streams.py           251    156    38%
source_salesforce/utils.py               8      7    12%
--------------------------------------------------------
TOTAL                                  484    236    51%
Name                                 Stmts   Miss  Cover
--------------------------------------------------------
source_salesforce/__init__.py            2      0   100%
source_salesforce/api.py               126     19    85%
source_salesforce/exceptions.py          1      0   100%
source_salesforce/rate_limiting.py      22      3    86%
source_salesforce/source.py             74      6    92%
source_salesforce/streams.py           251     43    83%
source_salesforce/utils.py               8      0   100%
--------------------------------------------------------
TOTAL                                  484     71    85%

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets February 7, 2022 10:35 Inactive
@antixar
Copy link
Contributor Author

antixar commented Feb 8, 2022

/test connector=connectors/source-salesforce

🕑 connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/1812211305
❌ connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/1812211305
🐛 https://gradle.com/s/q76rk3rjibuv2

@antixar antixar marked this pull request as ready for review February 8, 2022 12:22
@antixar antixar temporarily deployed to more-secrets February 8, 2022 12:25 Inactive
@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets February 8, 2022 12:26 Inactive
@antixar
Copy link
Contributor Author

antixar commented Feb 8, 2022

/test connector=connectors/source-salesforce

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

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/config.py                        74      6    92%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/tests/test_core.py              275    106    61%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/tests/test_incremental.py        69     38    45%
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/utils/common.py                  70     17    76%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/utils/connector_runner.py       110     48    56%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
------------------------------------------------------------------------
TOTAL                                                  876    259    70%
Name                                 Stmts   Miss  Cover
--------------------------------------------------------
source_salesforce/__init__.py            2      0   100%
source_salesforce/api.py               126     30    76%
source_salesforce/exceptions.py          1      0   100%
source_salesforce/rate_limiting.py      22      6    73%
source_salesforce/source.py             74     34    54%
source_salesforce/streams.py           254     50    80%
source_salesforce/utils.py               8      7    12%
--------------------------------------------------------
TOTAL                                  487    127    74%
Name                                 Stmts   Miss  Cover
--------------------------------------------------------
source_salesforce/__init__.py            2      0   100%
source_salesforce/api.py               126     19    85%
source_salesforce/exceptions.py          1      0   100%
source_salesforce/rate_limiting.py      22      3    86%
source_salesforce/source.py             74      6    92%
source_salesforce/streams.py           254     43    83%
source_salesforce/utils.py               8      0   100%
--------------------------------------------------------
TOTAL                                  487     71    85%

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets February 8, 2022 14:58 Inactive
@sherifnada
Copy link
Contributor

@antixar thanks for filling out the full PR description - its very helpful!

all failed jobs will be skipped and user will be able to know about this into Airbyte logs only.

this seems like an issue. Specifically, if data failed to be loaded, then the sync should fail, no? @antixar why was this behavior chosen for this PR?

@sherifnada
Copy link
Contributor

@antixar I think I understand the behavior now: if the job fails, we don't give up but rather use the REST stream. Is that correct? if so, then this looks good!

@antixar
Copy link
Contributor Author

antixar commented Feb 10, 2022

@antixar I think I understand the behavior now: if the job fails, we don't give up but rather use the REST stream. Is that correct? if so, then this looks good!

@sherifnada , you are right. The connector will try to call a REST request (with same necessary offsets and states etc) farther if the BULK logic returns an error.
But I think the REST logic will return an error anyways if the BULK returns same . But errors of failure from the REST endpoints are more understandable.

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Feb 10, 2022
@antixar antixar temporarily deployed to more-secrets February 10, 2022 17:23 Inactive
@antixar
Copy link
Contributor Author

antixar commented Feb 10, 2022

/publish connector=connectors/source-salesforce

🕑 connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/1825192660
✅ connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/1825192660

@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #10141   +/-   ##
=========================================
  Coverage          ?   85.42%           
=========================================
  Files             ?        7           
  Lines             ?      487           
  Branches          ?        0           
=========================================
  Hits              ?      416           
  Misses            ?       71           
  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 b230543...d05e67f. Read the comment docs.

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets February 10, 2022 17:27 Inactive
@antixar antixar temporarily deployed to more-secrets February 10, 2022 17:45 Inactive
@antixar antixar merged commit 5bc8ec2 into master Feb 10, 2022
@antixar antixar deleted the antixar/105-oncall-salesforce-failed-jobs branch February 10, 2022 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Salesforce: removal of the wait_timeout option
3 participants