Skip to content

Tap Shopify Performance Enhancements#34

Open
siddharthsudheer wants to merge 6 commits intosinger-io:masterfrom
siddharthsudheer:master
Open

Tap Shopify Performance Enhancements#34
siddharthsudheer wants to merge 6 commits intosinger-io:masterfrom
siddharthsudheer:master

Conversation

@siddharthsudheer
Copy link
Copy Markdown

No description provided.

@cmerrick
Copy link
Copy Markdown

cmerrick commented Apr 9, 2019

Hi @siddharthsudheer, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

@cmerrick
Copy link
Copy Markdown

cmerrick commented Apr 9, 2019

You did it @siddharthsudheer!

Thank you for signing the Singer Contribution License Agreement.

async with session.get(url=url, headers=headers, params=params) as response:
if response.status == 200:
return await response.json()
elif response.status == 429:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There appears to be a bug here with status codes != 200, 429, and 500-599. Getting a 401, for example will miss.

return ranges

async def _get_async(self, url, headers=None, params=None, retry_attempt=0):
headers = {**headers, "Connection": "close"} if headers else {"Connection": "close"}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is some missing code to connect this with the auth info. ie its missing the header: headers['X-Shopify-Access-Token']

import math
import functools
import datetime
from datetime import datetime, timedelta
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changing this import broke code down below datetime.timedelta(days=date_window_size)

@KAllan357
Copy link
Copy Markdown
Contributor

Thanks for this contribution. I added some comments because I was not able to get this working without changes.

I ran the code against our test data set (which syncs all tables including the non-async tables) and found no noticeable performance improvement. Any suggestions for figuring out what went wrong?

@airhorns
Copy link
Copy Markdown
Contributor

@siddharthsudheer are you interested in following through with this PR? It seems like it has the potential to make a huge difference if you have small order counts per day.

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.

4 participants