fix: Return None from add_request when storage client fails to enqueue request#1775
fix: Return None from add_request when storage client fails to enqueue request#1775Mantisus wants to merge 2 commits intoapify:masterfrom
None from add_request when storage client fails to enqueue request#1775Conversation
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
LGTM, thanks. One suggestion regarding the warning log. Also, it would be good if @janbuchar could check this as well, just in case.
| return response.processed_requests[0] | ||
|
|
||
| if response.unprocessed_requests: | ||
| logger.warning(f'Request {request.url} was not processed and returned as unprocessed.') |
There was a problem hiding this comment.
Instead of:
Request {request.url} was not processed and returned as unprocessed.
Could we update the warning log to something like:
Request {request.url} was not processed by storage client {storage_client}.
This would provide additional context by explicitly identifying the storage client as the component that did not process the request.
(I am not sure if we can manage to get the storage client name here; if not, leave it without it.
| *, | ||
| forefront: bool = False, | ||
| ) -> ProcessedRequest: | ||
| ) -> ProcessedRequest | None: |
There was a problem hiding this comment.
This is breaking on the type level, but the None is only returned in cases that would throw an error previously, so I guess it's fine. But it'd be nice if we mentioned it in the changelog or something.
3 participants
Description
RequestManager.add_requestto returnNonewhen the storage client fails to process the request and returns it inAddRequestsResponse.unprocessed_requests. Previously, accessingprocessed_requests[0]on an empty list caused anIndexError.Testing