blackducksoftware / hub-rest-api-python

HUB REST API Python bindings
Apache License 2.0
89 stars 104 forks source link

Updated Koshmack/dev filtering notifications to address HTTP error handling #233

Closed koshmack closed 1 year ago

koshmack commented 1 year ago

The suggested HTTP error handling for continued reading and updating the Notifications is added and tested. It is now ready to receive another round of review.

nichollsdave commented 1 year ago

If I read the changes to error handling correctly if there is an error at any point reading or updating the notifications then it will add one to the failure count, and try again and reload all notifications and start again. Therefore if there is a constant problem it will get stuck in that while True loop. I guess this is robust on one hand because it will keep trying but is there a point where you want it to give up, say after 100 errors? Not saying we definitely want that just wondering what the requirement is, would you prefer it to keep trying until it succeeds (or keep trying for eternity)?

OffBy0x01 commented 1 year ago

The defaults built into HubSession should be sufficient for HTTP retry events, was there a specific error you encountered where this was not sufficient?

koshmack commented 1 year ago

Hi, the motivation for the "additional" retries in this script was continuity with hope that the error was temporary and we could read and update more notifications until the end of the cached ones. However, after the 2nd thought of mine, I would prefer to revert to the original error handling which only depends on the hub python API's & Python urllib3's embedded retry (default is 15 sec * 3 times). If the customer's network quality is not excellent, they could increase the timer or the retry count or both which are configurable in .restconfig-notifications.json file for the adjustment. Besides, the original error handling is simple with no extra boilerplate. Thank you

koshmack commented 1 year ago

I will close this PR and will submit a new one where the error handling is reverted to the original one.