[Confluence] Improve error handling in Confluence pagination #2610
Conversation
There was a problem hiding this comment.
I wish you'd started a discussion about how you'd go about solving this, before making a whole PR. I feel bad that you put in this work, but I don't agree with the approach taken here. I'd prefer that you revisit the Acceptance Criteria on the linked ticket, and take the approach suggested there.
|
We appreciate your perspective and understand your preference for discussing the solution before implementing it. According to the Acceptance Criteria, current approach involves utilizing retryable for error handling and re-throwing errors if they are not resolved. Furthermore, a mechanism is implemented to manage errors in a manner that prevents a large synchronization from failing due to minor data issues. This is achieved by establishing a threshold of 30% for the number of indexed documents before triggering an error, striking a balance between the imperative of quick error detection ("fail fast") and the necessity to prevent catastrophic failures in large synchronization processes. |
…pagination-swallow-error
…pagination-swallow-error
|
buildkite test this |
|
@seanstory The PR is ready for review |
|
buildkite test this |
|
@artem-shelkovnikov @seanstory we have implemented the changes, can we have another round of review? |
…pagination-swallow-error
There was a problem hiding this comment.
Sorry for the delay, for some reason your comments did not show up in my github notifications.
This PR is still not in a mergable state. There are still numerous comments from the last review that were not responded to or closed, and it's unclear if they were considered at all.
This implementation raises more exceptions in some spots, swallows exceptions in other spots, but other than one place does not handle the errors at all. It is unclear to me how this benefits the end user or solves the ticket it is linked to.
If it is still unclear how to move forward with this, we will close this PR and implement it internally.
| known_errors = { | ||
| ServerConnectionError, | ||
| ClientResponseError, | ||
| ClientPayloadError, |
There was a problem hiding this comment.
Why is this in the list if there's a _handle_client_payload_errror() function?
There was a problem hiding this comment.
To re-raised the exception if error is not handled by _handle_client_payload_errror()
| async def _handle_client_payload_error(self, exception): | ||
| retry_seconds = DEFAULT_RETRY_SECONDS | ||
| response_headers = exception.headers or {} | ||
| if "Retry-After" in response_headers: |
There was a problem hiding this comment.
What about client payload errors makes it special and the only thing that should look at a retry-after header?
There was a problem hiding this comment.
Client payload errors often include a Retry-After header to manage rate limits and server load, indicating how long to wait before retrying, similar to how we handle it for OneDrive and SharePoint Online.
There was a problem hiding this comment.
But why not other error types too? Like 429 response codes should pretty much always include a Retry-After header
There was a problem hiding this comment.
You're right, 429 response codes should also include a Retry-After header. However, we already handle this logic in the _handle_client_errors(), which checks for the Retry-After header for 429 response code
connectors/sources/confluence.py
Outdated
| except ( | ||
| ServerConnectionError, | ||
| ClientResponseError, | ||
| ClientPayloadError, | ||
| Forbidden, | ||
| UnauthorizedException, | ||
| ThrottledError, | ||
| NotFound, | ||
| InternalServerError, | ||
| Exception, | ||
| ) as exception: |
There was a problem hiding this comment.
What's the purpose in having this whole list, but including Exception? Isn't this the same as
except Exception as exception:
?
…pagination-swallow-error
…tic/connectors into confluence-pagination-swallow-error
I've reviewed and addressed all previous comments. If I've missed anything, please let me know.
Ideally we are raising the exception in api_call method and the unknown exception is being swallowed if threshold is within the criteria and handling the exception where the api is being called. If you have any confusion or doubt redarding this please let us know
As per the certiera |
|
I'm going to close this PR in favor of #2903. @moxarth-elastic @parth-elastic if you'd like to contribute to that branch (it still needs unit tests), please go ahead. Otherwise, when I get to it, I'll re-assign the parent issue internally, and close it. |
Closes #2394
Checklists
Pre-Review Checklist
config.yml.example)v7.13.2,v7.14.0,v8.0.0)Changes Requiring Extra Attention
Related Pull Requests
Release Note