adobe-apiplatform / umapi-client.py

Python client for the User Management API (UMAPI) from Adobe
https://developer.adobe.com/umapi/
MIT License
12 stars 19 forks source link

Request handling #67

Closed vossen-adobe closed 5 years ago

vossen-adobe commented 5 years ago

Add session manager to handle requests in connection.py. Includes the new parameters:

connection_pooling=True # Use sessions (default client behavior) or plain requests.get/post/delete retry_cooldown=5 # How long to wait (s) before retrying call following unhandled exception session_max_age=1000 # Max age of a session before it is scrapped (s) - default roughly 15 minutes log_endpoint_ip=False # Log the UMAPI endpoint - useful for diagnosing failers. Default is disabled

All tests (unit and live) pass at time of this commit (attached). test_results.pdf

adorton-adobe commented 5 years ago

@janssenda-adoobe Thank you for taking a look at this. I'll review it by the end of next week. Will you please rebase your branch against this repo's master? It'll help ensure that only your changes are merged once the PR is approved. Let me know if you need more information - I've had to do it a few times.

vossen-adobe commented 5 years ago

@adorton-adobe I have rebased onto apiplatform/master (it doesn't show in the commit correctly)

adorton-adobe commented 5 years ago

Thank you for doing that. I'm still seeing the copyright notice and possibly other unrelated changes when I review changes, however. Did you force push your rebased branch? If not, you'll need to remove your commits related to the rebase, re-do the rebase, and force push it to your fork. The force push means that only your new commits are pushed, and you won't need to merge the rebased commits.

Sorry to be such a stickler on this, but I noticed some changes when I did an initial review that didn't seem related to the new functionality, and I'm not sure if they were part of an old commit that should be removed from the PR with a rebase.

If you need any help with this please let me know - I'd be happy to jump on a call to review it with you.

vossen-adobe commented 5 years ago

Hey @adobeDan - thanks for looking this over. A great deal of the formatting and blank line issues were due to pycharm as well as beginning with the released version from the pypi repo before actually getting to the correct source. I made an effort not to commit those changes along the way, but I clearly missed more than a few, If you think it would be better given the number of things here, I can close this PR and prepare a new branch from scratch to clean things up.

As for the request_handler - the intent was to skip the session initialization completely, making a simple request for the case when pooling is not enabled. This was the driving force for the PR, based on experience with the client. The requests library does make a brand new session under the hood for a call, so I saw no good reason to do this explicitly in the class level instead of letting requests handle it.

In the end, I kept the approach since it seemed to solve the issue we were having, but I did not have much chance to troubleshoot alternatives. If you think recreating the session every time is preferrable and will promote the same behavior, I'd be more than happy to refactor that down!

adobeDan commented 5 years ago

The point of making a new session explicitly is that it keeps the discipline of header and auth and timeout management consistent regardless of how often you are creating sessions. Since each request actually creates a session under the hood, there is no performance penalty, and a lot more control. For example, suppose the requests library itself decided to do session reuse by default: that would break the current code (which assumes it is not). Being explicit is always a better option.

vossen-adobe commented 5 years ago

Disregard last commit - I'm closing and that was meant for my fork only.