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

Make the async version of the client easier to use #79

Closed
wants to merge 2 commits into from

Conversation

wojcikstefan
Copy link
Member

@wojcikstefan wojcikstefan commented Jan 28, 2017

This is an alternative to #78 fixing some of the async client's flaws instead of removing it altogether.

Example usage:

In [1]: from closeio_api import Client

In [2]: api = Client('api_key', development=True, async=True)

In [3]: results = api.map([
   ...:   api.post('lead', {'name': 'New Lead 1'}),
   ...:   api.post('lead', {'name': 'New Lead 2'}),
   ...:   api.post('lead', {'name': 'New Lead 3'}),
   ...:   api.post('lead', {'name': 'New Lead 4'})
   ...: ])

In [4]: len(results)
Out[4]: 4

In [5]: results[0]['id']
Out[5]: u'lead_RpzCIwspNlTUowYUvIUaNE1tp3ncVFk8x3Uw4sv96FO'

I'm still conflicted whether such a deep integration of concurrency is good for this client library and its users or not. On the one hand, it makes concurrent requests easier to construct and send, and it handles API errors and retry logic properly (-ish). On the other hand, it makes the code more complex (and thus harder to read/understand), it forces a particular implementation of concurrency (green threads & gevent), and there are still some quirks with it (e.g. using the "debug" flag differs unintuitively between the sync and async client).

@closeio/engineering what do you think?

This PR introduces breaking changes and should be published along with a major version bump.


responses = [(
response.json() if response.ok else APIError(response)
) for response in self.requests.map(reqs)]
Copy link
Member

Choose a reason for hiding this comment

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

The pool needs to have a (safe) size to prevent firing too many concurrent API requests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 920782d

@thomasst
Copy link
Member

It seems weird that api.post() wouldn't actually post a request with an async client. Is there a less confusing syntax we could use?

@wojcikstefan
Copy link
Member Author

Hm, I actually thought that it's more intuitive than the alternative where an async client sends requests synchronously upon api.get/post/put/delete. Might be just my personal preference though...

An alternative could be to have api.prepare_get/prepare_post/etc. which would internally use _prepare_request.

@thomasst did you also take a look at #78 ?

# max_retries limit is reached
retries = 0
while True and retries < max_retries:
n_errors = sum([int(isinstance(response, APIError))
Copy link
Member

Choose a reason for hiding this comment

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

ValidationError inherits APIError. We should make sure it doesn't get retried though.

@wojcikstefan
Copy link
Member Author

Closing this PR as it seems we're all in consensus that such a specific async implementation within our library is not necessary. Will follow the path that PR #78 lays out instead.

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.

None yet

2 participants