Skip to content

Don't retry table metadata read when fails with ValidationException#17905

Merged
findepi merged 1 commit intotrinodb:masterfrom
krvikash:krvikash/iceberg-timeout
Jun 17, 2023
Merged

Don't retry table metadata read when fails with ValidationException#17905
findepi merged 1 commit intotrinodb:masterfrom
krvikash:krvikash/iceberg-timeout

Conversation

@krvikash
Copy link
Copy Markdown
Contributor

Description

Don't retry table metadata read when fails with ValidationException

@cla-bot cla-bot bot added the cla-signed label Jun 14, 2023
@krvikash krvikash force-pushed the krvikash/iceberg-timeout branch from f2e8fe0 to a32ea5d Compare June 14, 2023 17:26
@krvikash krvikash requested review from ebyhr, findepi and findinpath June 14, 2023 17:28
@github-actions github-actions bot added the iceberg Iceberg connector label Jun 14, 2023
@krvikash krvikash force-pushed the krvikash/iceberg-timeout branch from a32ea5d to 7b18ab6 Compare June 14, 2023 19:59
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.

// try to drop table
assertUpdate("DROP TABLE " + tableName);
assertFalse(getQueryRunner().tableExists(getSession(), tableName));
assertFalse(fileSystem.listFiles(tableLocation).hasNext(), "Table location should not exist");

Do you happen to remember why we've added the test for the situation on the smoke test and not on BaseIcebergConnectorTest ?
I'm not necessarily requesting the checks to be done here as well.

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.

The tests are also added in BaseIcebergConnectorTest.

We wanted to verify the corrupted table against all catalogs and smoke test runs against all catalogs so added smoke test.

Copy link
Copy Markdown
Contributor Author

@krvikash krvikash Jun 14, 2023

Choose a reason for hiding this comment

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

One more reason to add smoke connector test is to verify against the different file systems.

@krvikash krvikash force-pushed the krvikash/iceberg-timeout branch from 7b18ab6 to 37a5b97 Compare June 14, 2023 20:44
@krvikash krvikash force-pushed the krvikash/iceberg-timeout branch from 37a5b97 to 0a2022a Compare June 16, 2023 16:54
@krvikash
Copy link
Copy Markdown
Contributor Author

Thank you @findepi for the review. Addressed comments.

@findepi findepi merged commit 51b619b into trinodb:master Jun 17, 2023
@github-actions github-actions bot added this to the 420 milestone Jun 17, 2023
@krvikash krvikash deleted the krvikash/iceberg-timeout branch June 18, 2023 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed iceberg Iceberg connector

Development

Successfully merging this pull request may close these issues.

3 participants