Skip to content
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

Handle BigQuery error when listing tables #20832

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

nevillelyh
Copy link
Member

@nevillelyh nevillelyh commented Feb 24, 2024

Description

BigQuery listTables could throw an BigQueryException and cause a Trino INTERNAL_ERROR. This PR catches & wraps it in a more specific EXTERNAL error similar to listDatasetIds.

@cla-bot cla-bot bot added the cla-signed label Feb 24, 2024
@nevillelyh nevillelyh requested a review from ebyhr February 24, 2024 19:52
@github-actions github-actions bot added the bigquery BigQuery connector label Feb 24, 2024
Comment on lines +265 to +267
catch (BigQueryException e) {
throw new TrinoException(BIGQUERY_LISTING_TABLE_ERROR, "Failed to retrieve tables from BigQuery", e);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this change breaks the exception handling in BigQueryMetadata#listTablesInRemoteSchema method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I changed listTablesInRemoteSchema to catch TrinoException instead assuming the client.listTableIds() call in there is the only thing that could throw BigQueryException.

Copy link
Member

Choose a reason for hiding this comment

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

there's also toRemoteTable which can call listTableIds depending on what signature of toRemoteTable is called. But it does wrap BigQueryException into a TrinoException.

Copy link
Member Author

Choose a reason for hiding this comment

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

The toRemoteTable version called by listTablesInRemoteSchema doesn't call listTabeIds, since it just called it directly and passed the IDs as tableIds. So I think we're covered.

@ebyhr ebyhr added the no-release-notes This pull request does not require release notes entry label Feb 25, 2024
@nevillelyh nevillelyh force-pushed the neville/bq-list-tables-error branch from 772b280 to 5615e39 Compare February 25, 2024 01:49
@nevillelyh nevillelyh force-pushed the neville/bq-list-tables-error branch from 5615e39 to beba4da Compare February 27, 2024 15:58
@hashhar hashhar merged commit c9bc619 into trinodb:master Feb 28, 2024
18 checks passed
@hashhar hashhar removed the no-release-notes This pull request does not require release notes entry label Feb 28, 2024
@github-actions github-actions bot added this to the 440 milestone Feb 28, 2024
@hashhar hashhar added the no-release-notes This pull request does not require release notes entry label Feb 28, 2024
@nevillelyh nevillelyh deleted the neville/bq-list-tables-error branch February 29, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bigquery BigQuery connector cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

4 participants