Skip to content

Implement token refresh on BigQuery dts run operator to handle the heavy migration jobs over 1 hour#37538

Closed
okayhooni wants to merge 1 commit into
apache:mainfrom
okayhooni:bigquery_dts/refresh_token
Closed

Implement token refresh on BigQuery dts run operator to handle the heavy migration jobs over 1 hour#37538
okayhooni wants to merge 1 commit into
apache:mainfrom
okayhooni:bigquery_dts/refresh_token

Conversation

@okayhooni

@okayhooni okayhooni commented Feb 19, 2024

Copy link
Copy Markdown
Contributor

related issue:

releated PR:

Currently, when BigQuery Data Transfer job triggered by Airflow operator, exceeds one hour, it fails due to the expiration of the credential (default lifespan = 1 hour).

Access Denied: Table projectid:table_name: Permission bigquery.tables.get denied on table projectid:datasetid.table_name (or it may not exist).

So, I add the token refresh logic on DTS run operator. (additionally, add fallback logic to timeout parameter based on execution_timeout of Operator)

@boring-cyborg boring-cyborg Bot added area:providers provider:google Google (including GCP) related issues labels Feb 19, 2024
Comment on lines +374 to +382
current_tick_div, current_tick_mod = divmod(idx * interval, self.token_refresh_interval_seconds)
next_tick_div, next_tick_mod = divmod((idx + 1) * interval, self.token_refresh_interval_seconds)
if (current_tick_div < next_tick_div and 0 < next_tick_mod) or (
current_tick_mod == 0 and 1 <= current_tick_div
):
_ = self.hook.refresh_credentials()
self.log.info(
f"Credentials were refreshed on tick: idx={idx}, idx*interval={idx * interval} sec"
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other providers also requires refresh logic but seems like they have simpler implementation. For example:

@staticmethod
def _is_oauth_token_valid(token: dict, time_key="expires_on") -> bool:
"""
Check if an OAuth token is valid and hasn't expired yet.
:param sp_token: dict with properties of OAuth token
:param time_key: name of the key that holds the time of expiration
:return: true if token is valid, false otherwise
"""
if "access_token" not in token or token.get("token_type", "") != "Bearer" or time_key not in token:
raise AirflowException(f"Can't get necessary data from OAuth token: {token}")
return int(token[time_key]) > (int(time.time()) + TOKEN_REFRESH_LEAD_TIME)

Can you explain why we need complex logic here?

Also please extract the check validation (time handling) to it's own private function - it should be covered by unit tests

@okayhooni okayhooni Feb 19, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for quick and detailed review..!

Actually, I can't find the token validation method on base_google.py module, as like databricks_base.py module you mentioned.. If it has similar method based on Google sdk, it will be more simple and elegant way, as you said. But, it tasks more GCP API calls to check token validity on every loop (+ Is it possible to check token validity or refresh token with already expired credential token..?)

We experienced some error cases, even though the tasks take just over 30 minutes, not 1 hour, with same authorization error. (I don't know why..) so I decided to use the half of default token lifespan to refresh token.

@okayhooni

Copy link
Copy Markdown
Contributor Author

I got the same authorization error even after applying this refresh logic.. So, I close it.

@okayhooni okayhooni closed this Feb 20, 2024
@eladkal

eladkal commented Feb 20, 2024

Copy link
Copy Markdown
Contributor

cc @VladaZakharova maybe something worth investigating

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants