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.

Sign up for GitHub

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

Jump to bottom

🐛 Source Intercom: switching from scroll to standard endpoints #8637

Merged
merged 13 commits into from
Dec 10, 2021
Merged

🐛 Source Intercom: switching from scroll to standard endpoints #8637

merged 13 commits into from
Dec 10, 2021

Conversation

antixar
Copy link
Contributor

@antixar antixar commented Dec 8, 2021

What

At same time companies' "scroll" logic can be used by one script only.
OnCall https://github.com/airbytehq/oncall/issues/39

How

Implementation of the following new hybrid logic:

  1. Initially the stream companies tries to load data by "scroll" endpoints.
  2. try to wait a "scroll" request within a minute (3 attempts with delay 20,5 seconds) if "stroll" is busy by another script
  3. switch to using of the "standard" endpoint.

Recommended reading order

  1. x.java
  2. y.python
Updating a connector

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

@github-actions github-actions bot added the area/connectors Connector related issues label Dec 8, 2021
@antixar antixar changed the title 🐛 Source Intetcom: switching from scroll to standard endpoints 🐛 Source Intercom: switching from scroll to standard endpoints Dec 8, 2021
@antixar antixar temporarily deployed to more-secrets December 8, 2021 23:39 Inactive
@antixar antixar requested a review from sherifnada December 8, 2021 23:40
@antixar antixar self-assigned this Dec 8, 2021
@antixar antixar marked this pull request as ready for review December 8, 2021 23:40
sherifnada
sherifnada approved these changes Dec 10, 2021
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

it may make things a little faster to first request the non-scroll endpoints, read the total_count field and if >=10k then use the scroll endpoint, but not a significant improvement over this logic here.

@antixar @sherifnada
Update airbyte-integrations/connectors/source-intercom/source_interco…
8dd7f5a 
…m/source.py

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
@antixar antixar temporarily deployed to more-secrets December 10, 2021 10:32 Inactive
@antixar
Copy link
Contributor Author

antixar commented Dec 10, 2021

@sherifnada ,
The main problem is to the "scroll" endpoint can be busy by another script permanently. In such cases we have to use "non-scroll" one only no matter total_count value. My implementation has one flaw that sometimes the connector will force to wait 1 minute for switching to another endpoints.

@jrhizor jrhizor temporarily deployed to more-secrets December 10, 2021 10:42 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 10, 2021 16:02 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 10, 2021 16:29 Inactive
@antixar
Copy link
Contributor Author

antixar commented Dec 10, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-tiktok-marketing

🕑 connectors/source-tiktok-marketing https://github.com/airbytehq/airbyte/actions/runs/1564360139
❌ connectors/source-tiktok-marketing https://github.com/airbytehq/airbyte/actions/runs/1564360139
🐛 https://gradle.com/s/z7bhojasvjk5y

@jrhizor jrhizor temporarily deployed to more-secrets December 10, 2021 16:53 Inactive
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Dec 10, 2021
@antixar
Copy link
Contributor Author

antixar commented Dec 10, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-intercom run-tests=false

🕑 connectors/source-intercom https://github.com/airbytehq/airbyte/actions/runs/1564605664
✅ connectors/source-intercom https://github.com/airbytehq/airbyte/actions/runs/1564605664

@antixar antixar temporarily deployed to more-secrets December 10, 2021 18:03 Inactive
@antixar
Copy link
Contributor Author

antixar commented Dec 10, 2021

was published without tests because at that time there was some CI runners' failure. All tests were pass locally.

@jrhizor jrhizor temporarily deployed to more-secrets December 10, 2021 18:04 Inactive
@antixar antixar merged commit e43c53d into master Dec 10, 2021
@antixar antixar deleted the antixar/oncall-38-intercom-companies-sync-failure branch December 10, 2021 19:07
@antixar antixar temporarily deployed to more-secrets December 10, 2021 19:07 Inactive
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
@antixar @sherifnada @schlattk
🐛 Source Intercom: switching from scroll to standard endpoints (airby…
119709b 
…tehq#8637)

* backoff for companies scroll

* remove a unused companies stream property

* fix tests

* bump version

* update source_specs

* update scroll logic

* update tests

* Update airbyte-integrations/connectors/source-intercom/source_intercom/source.py

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>

* update change log

* update spec files

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
@lazebnyi lazebnyi mentioned this pull request Jan 14, 2022
Merged
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherifnada sherifnada sherifnada approved these changes

Assignees

@antixar antixar

Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@antixar @sherifnada @jrhizor