Skip to content

test: prevent alchemy test suite from being rate-limited. #1009

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

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 39 additions & 11 deletions tests/system/test_alembic.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
# CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

import contextlib
import time

import pytest
from sqlalchemy import Column, DateTime, Integer, String, Numeric
Expand Down Expand Up @@ -69,6 +70,7 @@ def test_alembic_scenario(alembic_table):

assert alembic_table("account") is None

_prevent_rate_limiting()
account = op.create_table(
"account",
Column("id", Integer, nullable=False),
Expand Down Expand Up @@ -96,6 +98,7 @@ def test_alembic_scenario(alembic_table):
{"description": None, "id": 3, "name": "savings"},
]

_prevent_rate_limiting()
op.add_column(
"account", Column("last_transaction_date", DateTime, comment="when updated")
)
Expand All @@ -107,6 +110,21 @@ def test_alembic_scenario(alembic_table):
SchemaField("last_transaction_date", "DATETIME", description="when updated"),
]

_prevent_rate_limiting()
op.rename_table("account", "accounts")
assert alembic_table("account") is None
assert alembic_table("accounts", "schema") == [
SchemaField("id", "INTEGER", "REQUIRED"),
SchemaField("name", "STRING(50)", "REQUIRED", description="The name"),
SchemaField("description", "STRING(200)"),
SchemaField("last_transaction_date", "DATETIME", description="when updated"),
]

_prevent_rate_limiting()
op.drop_table("accounts")
assert alembic_table("accounts") is None

_prevent_rate_limiting()
op.create_table(
"account_w_comment",
Column("id", Integer, nullable=False),
Expand All @@ -115,29 +133,23 @@ def test_alembic_scenario(alembic_table):
comment="This table has comments",
)
assert alembic_table("account_w_comment").description == "This table has comments"

_prevent_rate_limiting()
op.drop_table_comment("account_w_comment")
assert alembic_table("account_w_comment").description is None

_prevent_rate_limiting()
op.drop_column("account_w_comment", "description")
assert alembic_table("account_w_comment", "schema") == [
SchemaField("id", "INTEGER", "REQUIRED"),
SchemaField("name", "STRING(50)", "REQUIRED", description="The name"),
]

_prevent_rate_limiting()
op.drop_table("account_w_comment")
assert alembic_table("account_w_comment") is None

op.rename_table("account", "accounts")
assert alembic_table("account") is None
assert alembic_table("accounts", "schema") == [
SchemaField("id", "INTEGER", "REQUIRED"),
SchemaField("name", "STRING(50)", "REQUIRED", description="The name"),
SchemaField("description", "STRING(200)"),
SchemaField("last_transaction_date", "DATETIME", description="when updated"),
]
op.drop_table("accounts")
assert alembic_table("accounts") is None

_prevent_rate_limiting()
op.create_table(
"transactions",
Column("account", Integer, nullable=False),
Expand All @@ -146,23 +158,39 @@ def test_alembic_scenario(alembic_table):
bigquery_time_partitioning=TimePartitioning(field="transaction_time"),
)

_prevent_rate_limiting()
op.alter_column("transactions", "amount", nullable=True)
assert alembic_table("transactions", "schema") == [
SchemaField("account", "INTEGER", "REQUIRED"),
SchemaField("transaction_time", "DATETIME", "REQUIRED"),
SchemaField("amount", "NUMERIC(11, 2)"),
]

_prevent_rate_limiting()
op.create_table_comment("transactions", "Transaction log")
assert alembic_table("transactions").description == "Transaction log"

_prevent_rate_limiting()
op.drop_table("transactions")

# Another thing we can do is alter the datatype of a nullable column,
# if allowed by BigQuery's type coercion rules
_prevent_rate_limiting()
op.create_table("identifiers", Column("id", Integer))

_prevent_rate_limiting()
op.alter_column("identifiers", "id", type_=Numeric)
assert alembic_table("identifiers", "schema") == [SchemaField("id", "NUMERIC")]

_prevent_rate_limiting()
op.drop_table("identifiers")


def _prevent_rate_limiting():
"""
Adds a delay between operations to prevent exceeding the "Maximum rate of table metadata update operations per table".

The rate limit is set to a maximum of 5 operations per 10 seconds. By introducing a sleep of 2 seconds
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm somewhat surprised that our exponential backoff query job retry logic doesn't handle this already. Do we have a link to a failing test stacktrace?

Instead of sleep, I'd like for us to add the rate limit as an allowed job retry reason in

https://github.com/googleapis/python-bigquery/blob/d1161dddde41a7d35b30033ccbf6984a5de640bd/google/cloud/bigquery/retry.py#L76-L84

if we know that the error is safe to retry.

If we do go this route, let's add a link to https://cloud.google.com/bigquery/quotas#standard_tables where this limit is defined.

Copy link
Contributor Author

@nlenepveu nlenepveu Jan 18, 2024

Choose a reason for hiding this comment

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

@tswast

Sure, it would be better handled in the python-bigquery code.. The work around I suggested is a quite quick and dirty solution..

Here's a stacktrace of the error https://source.cloud.google.com/results/invocations/ffafb866-6bc0-423f-a86b-df69fb270d57/log.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've filed googleapis/python-bigquery#1790 as the preferred approach to fixing this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Thanks for taking the time to describe the issue.

Let's see if we close this one or want to keep a workaround in the meantime.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am inclined to see if we can get this fixed.
Will work through things on the Issue Tim created...googleapis/python-bigquery#1790

before each operation, this function helps to ensure that the rate limit is not exceeded.
"""
time.sleep(2)