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

Resolving issue #1353: implement backoff in Google Sheets #1438

Merged
merged 6 commits into from
Dec 31, 2020
Merged

Resolving issue #1353: implement backoff in Google Sheets #1438

merged 6 commits into from
Dec 31, 2020

Conversation

yevhenii-ldv
Copy link
Contributor

@yevhenii-ldv yevhenii-ldv commented Dec 23, 2020

What

Using a backoff in the Google Sheets source code when the request limit is reached.

How

Using python library backoff

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

Recommended reading order

  1. test.java
  2. component.ts
  3. the rest

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.

@yevhenii-ldv This looks great so far, but we aren't backing off on every client call. For example in the discovery method we make a call using client but there is no backing off there as far as i can tell. Could we add this backoff to every call made with the client? It may make sense to put the client in a separate class if it helps.

@yevhenii-ldv
Copy link
Contributor Author

@sherifnada PR was updated with new client`s class for Google Sheets requests with backoff
Merry Christmas))

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.

Merry christmas @yevhenii-ldv! looking much better already. Two more things needed:

  1. Update the version of this connector in the Dockerfile, YAML, and generated JSON file. We generally want to do this whenever we make an update to a connector so we can release a new version.
  2. small structural change to the client

@sherifnada sherifnada mentioned this pull request Dec 25, 2020
@yevhenii-ldv
Copy link
Contributor Author

yevhenii-ldv commented Dec 30, 2020

@sherifnada I have updated the PR as per your comments.

Copy link
Contributor

@jrhizor jrhizor left a comment

Choose a reason for hiding this comment

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

Looks good!

I'm going to build the image, run the tests, and then merge this.

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.

3 participants