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

gsheetsImport: reduce the number of API calls #526

Merged
merged 3 commits into from
Apr 27, 2020
Merged

gsheetsImport: reduce the number of API calls #526

merged 3 commits into from
Apr 27, 2020

Conversation

alanb-sony
Copy link
Contributor

Uploading test results from multiple platforms can hit the google API rate limits. This patch reduces the number of reads and writes made so should reduce the chance of hitting the limits.

garethsb
garethsb reviewed Apr 20, 2020
View reviewed changes
utilities/run-test-suites/gsheetsImport/resultsImporter.py
return False
return True


def gsheets_import(test_results, worksheet, filename, start_col=1):
"""Upload results data to spreadsheet"""

Copy link
Contributor

@garethsb garethsb Apr 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if we should be using worksheet.get_all_values('FORMULA') given we're using 'USER_ENTERED' later?

@garethsb garethsb requested a review from andrewbonney April 20, 2020 17:19
@garethsb garethsb changed the title reduce the number of API calls gsheetsImport: reduce the number of API calls Apr 20, 2020
@garethsb
Copy link
Contributor

Going to look at using append_rows (or insert_row(s), after concerns mentioned in burnash/gspread#734 have been addressed to make that atomic) to reduce API usage further. Using insert_row(s) would enable us to add a command-line flag to insert at the top so in large spreadsheets of historical results new stuff is easy to find.

@garethsb garethsb marked this pull request as draft April 24, 2020 07:12
@garethsb garethsb self-requested a review April 24, 2020 07:12
@burnash
Copy link

burnash commented Apr 27, 2020

@alanb-sony would you be interested in contributing your atomic version of insert_rows() to gspread?

@alanb-sony
add option to insert new rows at the top of the sheets
3b76b29 
use append to add rows at the bottom of the sheet to fix race conditions on concurrent updates
@alanb-sony
Copy link
Contributor Author

@burnash yes if you think the implementation is reasonable?

@burnash
Copy link

burnash commented Apr 27, 2020

I think so. Would it be possible to contribute a multi-rows version of the implementation in this PR?
gspread already has insert_row(). I've merged burnash/gspread#734 with insert_rows() but haven't released it yet. I believe if there's a chance to reduce the number of API calls we should certainly use it.

garethsb
garethsb approved these changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@garethsb garethsb left a comment

Choose a reason for hiding this comment

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

LGTM

@garethsb garethsb marked this pull request as ready for review April 27, 2020 16:30
@garethsb garethsb merged commit e6e3f63 into AMWA-TV:master Apr 27, 2020
@garethsb garethsb mentioned this pull request Apr 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garethsb garethsb garethsb approved these changes

@andrewbonney andrewbonney Awaiting requested review from andrewbonney

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alanb-sony @garethsb @burnash