-
Notifications
You must be signed in to change notification settings - Fork 3
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance HexRunProjectOperator and HexHook #21
Open
jacobcbeaudin
wants to merge
2
commits into
hex-inc:main
Choose a base branch
from
jacobcbeaudin:jacobcbeaudin/enhance-hex-operator
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
0.1.9 | ||
0.1.10 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
"""Version information for the package.""" | ||
|
||
import os | ||
import sys | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,8 @@ | |
from airflow.exceptions import AirflowException | ||
from airflow.hooks.base import BaseHook | ||
from importlib_metadata import PackageNotFoundError, version | ||
from requests.exceptions import RequestException | ||
from tenacity import retry, stop_after_attempt, wait_fixed | ||
|
||
from airflow_provider_hex.types import NotificationDetails, RunResponse, StatusResponse | ||
|
||
|
@@ -151,52 +153,74 @@ def run_project( | |
), | ||
) | ||
|
||
def run_status(self, project_id, run_id) -> StatusResponse: | ||
@retry(stop=stop_after_attempt(3), wait=wait_fixed(1)) | ||
def run_status(self, project_id: str, run_id: str) -> StatusResponse: | ||
endpoint = f"api/v1/project/{project_id}/run/{run_id}" | ||
method = "GET" | ||
try: | ||
response = self.run(method=method, endpoint=endpoint, data=None) | ||
return cast(StatusResponse, response) | ||
except RequestException as e: | ||
self.log.error(f"API call failed: {str(e)}") | ||
raise | ||
|
||
return cast( | ||
StatusResponse, self.run(method=method, endpoint=endpoint, data=None) | ||
) | ||
|
||
def cancel_run(self, project_id, run_id) -> str: | ||
def cancel_run(self, project_id: str, run_id: str) -> str: | ||
endpoint = f"api/v1/project/{project_id}/run/{run_id}" | ||
method = "DELETE" | ||
|
||
self.run(method=method, endpoint=endpoint) | ||
return run_id | ||
|
||
def run_and_poll( | ||
def run_status_with_retries( | ||
self, project_id: str, run_id: str, max_retries: int = 3, retry_delay: int = 1 | ||
) -> StatusResponse: | ||
@retry(stop=stop_after_attempt(max_retries), wait=wait_fixed(retry_delay)) | ||
def _run_status(): | ||
return self.run_status(project_id, run_id) | ||
|
||
return _run_status() | ||
|
||
def poll_project_status( | ||
self, | ||
project_id: str, | ||
inputs: Optional[dict], | ||
update_cache: bool = False, | ||
run_id: str, | ||
poll_interval: int = 3, | ||
poll_timeout: int = 600, | ||
kill_on_timeout: bool = True, | ||
notifications: List[NotificationDetails] = [], | ||
): | ||
run_response = self.run_project(project_id, inputs, update_cache, notifications) | ||
run_id = run_response["runId"] | ||
|
||
max_poll_retries: int = 3, | ||
poll_retry_delay: int = 5, | ||
) -> StatusResponse: | ||
poll_start = datetime.datetime.now() | ||
while True: | ||
run_status = self.run_status(project_id, run_id) | ||
try: | ||
run_status = self.run_status_with_retries( | ||
project_id, run_id, max_poll_retries, poll_retry_delay | ||
) | ||
except Exception as e: | ||
self.log.error( | ||
f"Failed to get run status after {max_poll_retries} " | ||
f"attempts: {str(e)}" | ||
) | ||
if kill_on_timeout: | ||
self.cancel_run(project_id, run_id) | ||
raise AirflowException( | ||
"Failed to get run status for project " | ||
f"{project_id} with run: {run_id}" | ||
) | ||
|
||
project_status = run_status["status"] | ||
|
||
self.log.info( | ||
f"Polling Hex Project {project_id}. Status: {project_status}." | ||
) | ||
if project_status not in VALID_STATUSES: | ||
raise AirflowException(f"Unhandled status: {project_status}") | ||
Comment on lines
-190
to
-191
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we want to remove this exception There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add this back |
||
|
||
if project_status == COMPLETE: | ||
break | ||
return run_status | ||
|
||
if project_status in TERMINAL_STATUSES: | ||
raise AirflowException( | ||
f"Project Run failed with status {project_status}. " | ||
f"See Run URL for more info {run_response['runUrl']}" | ||
f"See Run URL for more info {run_status['runUrl']}" | ||
) | ||
|
||
if ( | ||
|
@@ -217,4 +241,28 @@ def run_and_poll( | |
) | ||
|
||
time.sleep(poll_interval) | ||
return run_status | ||
|
||
def run_and_poll( | ||
self, | ||
project_id: str, | ||
inputs: Optional[dict], | ||
update_cache: bool = False, | ||
poll_interval: int = 3, | ||
poll_timeout: int = 600, | ||
kill_on_timeout: bool = True, | ||
notifications: List[NotificationDetails] = [], | ||
max_poll_retries: int = 3, | ||
poll_retry_delay: int = 5, | ||
): | ||
run_response = self.run_project(project_id, inputs, update_cache, notifications) | ||
run_id = run_response["runId"] | ||
|
||
return self.poll_project_status( | ||
project_id, | ||
run_id, | ||
poll_interval, | ||
poll_timeout, | ||
kill_on_timeout, | ||
max_poll_retries, | ||
poll_retry_delay, | ||
) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
A retry limit should be enforced server-side to prevent the caller from specifying an excessive number of retries for project runs, which could lead to resource exhaustion or unintended behavior.
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 implementing a server-side retry limit currently prioritized? As this PR focuses on client-side Airflow hooks, I can hardcode a conservative number of retries and delay to prevent configurability. This would address immediate concerns while keeping the implementation on the client side. Let me know if you'd like me to proceed with this approach.
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.
Sorry for the delay here! (Also this is a product-manager translation from an engineer, so apologies if I lose something in translation!)
We rate limit requests, so at this time don't intend to implement a server-side retry limit. Also, our API is fairly non-flakey, so we don't think users will need to retry that often.
That being said, it would still be great to hardcode a conservative number of retries juuuust in case —
3
seems like a good spot!