- Notifications
You must be signed in to change notification settings - Fork126
[PECOBLR-782] added retry handling based on idempotency#689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for your contribution! To satisfy the DCO policy in ourcontributing 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 ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks okay overall.
iIs the test coverage already good? Because I don't see any new tests added
| # Mapping of CommandType to CommandIdempotency | ||
| # Based on the official idempotency classification | ||
| COMMAND_IDEMPOTENCY_MAP= { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what comes under CommandType.OTHER? Please clarify
| ifnotself._is_method_retryable(method): | ||
| returnFalse,"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
comment mentions Request was a GetOperationStatus but in if condition you're checking for all metadata command types. Is this intended?
| returnFalse,"Received code 501 from server." | ||
| # Request failed and this method is not retryable. We only retry POST requests. | ||
| ifnotself._is_method_retryable(method): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
where is this method defined?
| "CloseOperation received 404 code from Databricks. Cursor is already closed." | ||
| ) | ||
| ifstatus_codeinNON_RETRYABLE_STATUS_CODES: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 | ||
| """ | ||
| withmocked_server_response(status=404)asmock_obj: | ||
| withmocked_server_response(status=429)asmock_obj: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nice catch!
you can run to fix lint issues |
| ifnotrows: | ||
| # Read all the rows, row_count should match | ||
| self.assertEqual(n,row_count) | ||
| assertn==row_count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is this change related to this PR
| finally: | ||
| # Reset command type after request completion to prevent it from affecting subsequent requests | ||
| ifisinstance(self._retry_policy,DatabricksRetryPolicy): | ||
| self._retry_policy.command_type=None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This again is imposing thread safety concerns. we need to figure out a way to avoid having a state inhttpclient.
Uh oh!
There was an error while loading.Please reload this page.
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