[PECOBLR-782] added retry handling based on idempotency #689
Conversation
nishkarsh-db
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
nishkarsh-db
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
nishkarsh-db
changed the title
|
|
||
| # Mapping of CommandType to CommandIdempotency | ||
| # Based on the official idempotency classification | ||
| COMMAND_IDEMPOTENCY_MAP = { |
There was a problem hiding this comment.
link the retry design doc section here that classifies requests
| CommandType.TELEMETRY_PUSH: CommandIdempotency.IDEMPOTENT, | ||
| CommandType.VOLUME_GET: CommandIdempotency.IDEMPOTENT, | ||
| CommandType.VOLUME_DELETE: CommandIdempotency.IDEMPOTENT, | ||
| CommandType.OTHER: CommandIdempotency.IDEMPOTENT, |
There was a problem hiding this comment.
what comes under CommandType.OTHER? Please clarify
| if not self._is_method_retryable(method): | ||
| return False, "Only POST requests are retried" | ||
|
|
||
| # Request failed with 404 and was a GetOperationStatus. This is not recoverable. Don't retry. |
There was a problem hiding this comment.
comment mentions Request was a GetOperationStatus but in if condition you're checking for all metadata command types. Is this intended?
| return False, "Received code 501 from server." | ||
|
|
||
| # Request failed and this method is not retryable. We only retry POST requests. | ||
| if not self._is_method_retryable(method): |
There was a problem hiding this comment.
where is this method defined?
| "CloseOperation received 404 code from Databricks. Cursor is already closed." | ||
| ) | ||
|
|
||
| if status_code in NON_RETRYABLE_STATUS_CODES: |
There was a problem hiding this comment.
I think we should return more descriptive error message, based on status_code.
eg: UNAUTHORIZED for 403, BAD_REQUEST for 400 etc
| before raising an exception | ||
| """ | ||
| with mocked_server_response(status=404) as mock_obj: | ||
| with mocked_server_response(status=429) as mock_obj: |
|
you can run to fix lint issues |
| if not rows: | ||
| # Read all the rows, row_count should match | ||
| self.assertEqual(n, row_count) | ||
| assert n == row_count |
There was a problem hiding this comment.
is this change related to this PR
| finally: | ||
| # Reset command type after request completion to prevent it from affecting subsequent requests | ||
| if isinstance(self._retry_policy, DatabricksRetryPolicy): | ||
| self._retry_policy.command_type = None |
There was a problem hiding this comment.
isn't setting command_type to NOT_SET better? it will be type safe
|
|
||
| try: | ||
| # Set command type for OAuth configuration request | ||
| self.http_client.setRequestType(CommandType.AUTH) |
There was a problem hiding this comment.
This again is imposing thread safety concerns. we need to figure out a way to avoid having a state in httpclient.
What type of PR is this?
Description
Refactored the retry handling mechanism to be idempotency based. Also fixed some tests (which were expecting 404 requests to be retried).
How is this tested?
Related Tickets & Documents