[IDEV-2771] RTUF stream responses from the server rather than collecting it in memory; Update CLI #180
Conversation
|
LGTM! Good job @briluza ! |
| ) | ||
| if should_wait: | ||
| log.info(f"Sleeping for {wait_for}s.") | ||
| time.sleep(wait_for) |
There was a problem hiding this comment.
This sleep is going to happen after a subsequent GET request has already been made. This will likely cause a timeout.
There was a problem hiding this comment.
ohh nice catch here! should be above before making request so every connection is fresh
|
|
||
| should_wait = ( | ||
| wait_for | ||
| and wait_for > 0 |
There was a problem hiding this comment.
Why are you checking if it is greater than 0? If wait_for is 0 then it will be False and checked in the previous line. I don't think wait_for can be negative given that it is the difference of the safe_after and now.
| should_wait = ( | ||
| wait_for | ||
| and wait_for > 0 | ||
| and not (self.api.rate_limit and (self.product == "account-information")) |
There was a problem hiding this comment.
The not self.api.rate_limit is already checked in the _wait_time() call. Why check it here again? And how can the product be "account-information" in a class specifically only used for the Feeds products?
There was a problem hiding this comment.
Checking this again, this should be removed as this was from the base class and this is specific for feeds.
| yield line | ||
| except Exception as e: | ||
| self.latest_feeds_status_code = 500 | ||
| yield {"status_ready": True, "error": str(e)} |
There was a problem hiding this comment.
If an exception occurs, this line will be yielded to the user. Is this what is desired?
There was a problem hiding this comment.
answer is related to my comment here #180 (comment)
| for line in response.iter_lines(): | ||
| if line: | ||
| yield line | ||
| except Exception as e: |
There was a problem hiding this comment.
Rather than a blanket Exception don't you think it will be more useful to the user to capture specific httpx exceptions: https://www.python-httpx.org/exceptions/ so the error messages are more specific? If the exception happens during iter_lines() above. The "FATAL: Failed to start the feed generator in _make_request. Reason:" message will be shown, but it will be inaccurate because the iteration already started and the user already has jsonlines yielded to them.
Also if you catch HTTPStatusError you can use raise_for_status() so then you don't have to do the unusual thing of setting self.latest_feeds_status_code, yielding a throwaway line, and then having self.setStatus(self.latest_feeds_status_code) in _get_results() check the status.
There was a problem hiding this comment.
thanks, this answer the yield part on exception but rather doing that, I'll set the status and throw the exception as well to the user.
Aslo when calling setStatus, if its not a success status code then it will throw the exception and it will stop the execution before yielding back to user.
| yield {"status_ready": True} | ||
|
|
||
| for line in response.iter_lines(): | ||
| if line: |
There was a problem hiding this comment.
You don't need this check. There should never be a double \n in the RTTF responses. And if there were, it shouldn't be hidden from the user.
There was a problem hiding this comment.
ahh okay, thanks for the info!
| user-agent: | ||
| - python-httpx/0.28.1 | ||
| x-api-key: | ||
| - 4b02d-a4719-e33e7-93128-5a5ff |
There was a problem hiding this comment.
You should remove this api_key from this repo.
| """ | ||
|
|
||
| from itertools import chain | ||
| import json |
| from itertools import zip_longest | ||
| from itertools import zip_longest, chain | ||
| from typing import Generator | ||
| from httpx import Client |
| Highlevel process: | ||
| httpx stream -> yield each json line -> check status code -> yield back data to client |
There was a problem hiding this comment.
I don't think this sequence is right. Isn't it actually:
httpx stream -> check status code -> yield back data to client -> repeat if 206
There was a problem hiding this comment.
🤦 thanks for spotting this! redunant 'yield'
Changes