Optimize backoff handling and improve error handling in api_get_post #13576
Conversation
Ludusm11
commented
Oct 1, 2024
Ludusm11
commented
- Improved the lock usage and backoff handling by ensuring sleep only occurs for positive durations.
- Replaced try/except block with .get() for safer and more efficient dictionary access to 'last_edit_date'.
- Minor improvements to code readability and maintainability without altering core functionality.
- Improved the lock usage and backoff handling by ensuring sleep only occurs for positive durations. - Replaced try/except block with .get() for safer and more efficient dictionary access to 'last_edit_date'. - Minor improvements to code readability and maintainability without altering core functionality.
|
These changes look good to me. Have you done any testing of this? |
|
I'll be happy to merge this if you fix the spacing errors. Is this an attempt to get Hacktoberfest approval or where are you coming from? |
| @@ -50,14 +50,16 @@ def __setitem__(self, key, value): | |||
| setattr(self, key, value) | |||
|
|
|||
| def __getitem__(self, item): | |||
| getattr(self, item) | |||
| return getattr(self, item) | |||
There was a problem hiding this comment.
This correction fixes a clear bug. Thank you.
|
|
||
|
|
||
| def api_get_post(post_url): | ||
| with GlobalVars.api_request_lock: | ||
| # Respect backoff, if we were given one | ||
| if GlobalVars.api_backoff_time > time.time(): | ||
| time.sleep(GlobalVars.api_backoff_time - time.time() + 2) | ||
| # Use max(0, ...) to ensure no negative sleep times | ||
| time.sleep(max(0, GlobalVars.api_backoff_time - time.time() + 2)) |
There was a problem hiding this comment.
While not wrong, the only ways for this to be needed is
- if it takes > 2 seconds between the
ifin the previous line and the call totime.sleep()or - if
GlobalVars.api_backoff_timechanges between the two statements. I'd have to double-check everywhere else in the code, but we should be using theGlobalVars.api_request_lockto lock any changes toGlobalVars.api_backoff_time, so that shouldn't be an issue.
Overall, I'd probably rather not have this guard against a negative value here and have the normal exception raised when/if the value passed to time.sleep() is negative, because if the value is negative, then something has probably gone wrong somewhere and we should be made aware, by the exception, that there's an issue somewhere, even if that causes a reboot (which would result in a safe recovery).
| time.sleep(GlobalVars.api_backoff_time - time.time() + 2) | ||
| # Use max(0, ...) to ensure no negative sleep times | ||
| time.sleep(max(0, GlobalVars.api_backoff_time - time.time() + 2)) | ||
|
|
There was a problem hiding this comment.
As with all of the additional blank lines added in this commit, please don't change the style used when making other changes, particularly when not a primary contributor the project. While I wouldn't object to some additional blank lines in this file, the current style in this file uses blank lines very sparingly. Please maintain the current style.
If style changes are to be made, they would ideally be made in a separate commit, even if in the same PR.
Incidental changes would reasonable, but the changes here are a definite change it the style used in this file, which really should be separate.
| post_data.last_edit_date = item['last_edit_date'] | ||
| except KeyError: | ||
| post_data.last_edit_date = post_data.creation_date # Key not present = not edited | ||
|
|
There was a problem hiding this comment.
If you are making a style change, this blank line is inconsistent with the new style. If you are making a change, please keep it consistent.
| except KeyError: | ||
| post_data.last_edit_date = post_data.creation_date # Key not present = not edited | ||
|
|
||
| post_data.last_edit_date = item.get('last_edit_date', post_data.creation_date) # Use .get() for safer access |
There was a problem hiding this comment.
This line is a direct equivalent to the try/except block which it is replacing. There's significant discussion in the Python world as to which is "better", for various values of "better". Personally, I prefer the .get(), but not enough to have previously pushed the change, but I wouldn't object to it being changed.
However, I would retain the functional comment that's in the old code, rather than have the comment that's been added here, which is just a comment on how to write code. In other words, the old comment is a reasonable code comment. The added comment is something that would be more appropriate in a commit comment instead of a code comment.
