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

Suppress permission denied when listing materialized views in Iceberg Glue #15893

Merged
merged 2 commits into from
Feb 4, 2023

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Jan 30, 2023

Description

Suppress permission denied when listing materialized views in Iceberg Glue
Similar fixes as #14971

Release notes

(x) Release notes are required, with the following suggested text:

# Iceberg
* Fix failure when access denied exceptions happened during listing materialized views in a Glue. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Jan 30, 2023
@ebyhr
Copy link
Member Author

ebyhr commented Jan 30, 2023

suite-delta-lake-databricks104 & 113 hit #13199

@ebyhr ebyhr requested review from findinpath and findepi and removed request for findinpath January 30, 2023 02:33
@ebyhr ebyhr self-assigned this Jan 30, 2023
stats.getGetTables())
.map(GetTablesResult::getTableList)
.flatMap(List::stream)
.filter(table -> isTrinoMaterializedView(table.getTableType(), firstNonNull(table.getParameters(), ImmutableMap.of())))
Copy link
Member

Choose a reason for hiding this comment

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

nit: this will conflict with #15909

@@ -771,32 +770,32 @@ private void updateView(ConnectorSession session, SchemaTableName viewName, Conn
@Override
public List<SchemaTableName> listMaterializedViews(ConnectorSession session, Optional<String> namespace)
{
ImmutableList.Builder<SchemaTableName> materializedViews = ImmutableList.builder();
Copy link
Member

Choose a reason for hiding this comment

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

Use normal for loop in TrinoGlueCatalog.listMaterializedViews

seems like switching to a for loop isn't really necessary

why not add additional catch condition to the try catch within flatMap?

Copy link
Member Author

@ebyhr ebyhr Jan 31, 2023

Choose a reason for hiding this comment

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

I feel a for loop is cleaner than an additional try catch within flatMap.

Copy link
Member

Choose a reason for hiding this comment

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

but there already was a catch. you need both in-loop and out-of-loop try-catches anyway, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought you pointed another flatMap(List::stream). The AccessDeniedException exception happened in the subsequent collect() method when I confirmed previously.

Copy link
Member

Choose a reason for hiding this comment

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

code is worth thousand words... i pushed a commit showing what i have in mind.
please let me know wdyt

Copy link
Member Author

Choose a reason for hiding this comment

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

I reconfirmed the failed place was computeNext() method in AwsSdkUtil#getPaginatedResults. It causes the failure outside of the current catch block.

                        catch (EntityNotFoundException | AccessDeniedException e) {
                            // Namespace may have been deleted or permission denied
                            return Stream.empty();
                        }
                    })
                    .collect(toImmutableList()); // AccessDeniedException happened here

Sorry, I should have written more detailed information in the commit message.

ebyhr added 2 commits February 2, 2023 09:01
Otherwise, upcoming AccessDeniedException would be
caught by outer catch block.
@ebyhr ebyhr force-pushed the ebi/iceberg-glue-information-schema branch from 5421fee to c1a4aab Compare February 2, 2023 00:13
@ebyhr
Copy link
Member Author

ebyhr commented Feb 2, 2023

Rebased on upstream to resolve conflicts.

@ebyhr ebyhr force-pushed the ebi/iceberg-glue-information-schema branch from e7f1b7a to c1a4aab Compare February 3, 2023 22:44
@ebyhr
Copy link
Member Author

ebyhr commented Feb 4, 2023

CI hit #14441

@ebyhr ebyhr merged commit 4d83020 into master Feb 4, 2023
@ebyhr ebyhr deleted the ebi/iceberg-glue-information-schema branch February 4, 2023 09:46
@ebyhr ebyhr mentioned this pull request Feb 4, 2023
@github-actions github-actions bot added this to the 407 milestone Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants