Correctly categorize external errors in Iceberg REST catalog #24828
+202
−55
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mayankvadariya
force-pushed
the
mayank/rest-catalog-exception
branch
from
a4df018 to
0444c4e
Compare
mayankvadariya
force-pushed
the
mayank/rest-catalog-exception
branch
from
0444c4e to
14b5be2
Compare
pajaks
reviewed
Jan 29, 2025
| @@ -233,6 +234,10 @@ public void testDropTableWithMissingMetadataFile() | |||
| public void testDropTableWithMissingSnapshotFile() | |||
| { | |||
| assertThatThrownBy(super::testDropTableWithMissingSnapshotFile) | |||
| .isInstanceOf(QueryFailedException.class) | |||
| .cause() | |||
There was a problem hiding this comment.
Can we check that cause is TrinoException?
There was a problem hiding this comment.
I had tried it but it fails with some error.
.cause()
.isInstanceOf(TrinoException.class)
java.lang.AssertionError:
Expecting actual throwable to be an instance of:
io.trino.spi.TrinoException
but was:
io.trino.spi.TrinoException: Failed to drop table: test_drop_table_with_missing_snapshot_file_0noolj73ah
at io.trino.plugin.iceberg.catalog.rest.TrinoRestCatalog.dropTable(TrinoRestCatalog.java:464)
at io.trino.plugin.iceberg.IcebergMetadata.dropTable(IcebergMetadata.java:2284)
at io.trino.plugin.base.classloader.ClassLoaderSafeConnectorMetadata.dropTable(ClassLoaderSafeConnectorMetadata.java:451)
...(16 remaining lines not displayed - this can be changed with Assertions.setMaxStackTraceElementsDisplayed)
pajaks
reviewed
Jan 29, 2025
| return restSessionCatalog.listNamespaces(convert(session)).stream() | ||
| .map(this::toSchemaName) | ||
| .collect(toImmutableList()); | ||
| try { |
There was a problem hiding this comment.
If try would include also collectNamespaces exception handling inside would not be necessary
There was a problem hiding this comment.
I kept it separately in case collectNamespaces is to be called from some other method.
| @@ -251,10 +274,24 @@ public List<TableInfo> listTables(ConnectorSession session, Optional<String> nam | |||
|
|
|||
| ImmutableList.Builder<TableInfo> tables = ImmutableList.builder(); | |||
| for (Namespace restNamespace : namespaces) { | |||
| listTableIdentifiers(restNamespace, () -> restSessionCatalog.listTables(sessionContext, toRemoteNamespace(session, restNamespace))).stream() | |||
| listTableIdentifiers(restNamespace, () -> { | |||
There was a problem hiding this comment.
Can we have one catch for whole method?
There was a problem hiding this comment.
I kept it separately to throw different error messages.
wendigo
reviewed
Jan 29, 2025
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java
Outdated
Show resolved
Hide resolved
wendigo
reviewed
Jan 29, 2025
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java
Outdated
Show resolved
Hide resolved
wendigo
reviewed
Jan 29, 2025
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java
Outdated
Show resolved
Hide resolved
wendigo
reviewed
Jan 29, 2025
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java
Outdated
Show resolved
Hide resolved
wendigo
reviewed
Jan 29, 2025
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java
Outdated
Show resolved
Hide resolved
wendigo
reviewed
Jan 29, 2025
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java
Outdated
Show resolved
Hide resolved
wendigo
approved these changes
Jan 29, 2025
mayankvadariya
force-pushed
the
mayank/rest-catalog-exception
branch
2 times, most recently
from
a673083 to
593f5c6
Compare
wendigo
reviewed
Jan 30, 2025
| @@ -191,7 +206,7 @@ public void dropNamespace(ConnectorSession session, String namespace) | |||
| throw new SchemaNotFoundException(namespace); | |||
| } | |||
| catch (RESTException e) { | |||
| throw new TrinoException(ICEBERG_CATALOG_ERROR, format("Failed to drop namespace: %s", namespace), e); | |||
| throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to drop namespace: '%s'".formatted(namespace), e); | |||
| @@ -208,6 +223,9 @@ public Map<String, Object> loadNamespaceMetadata(ConnectorSession session, Strin | |||
| catch (NoSuchNamespaceException e) { | |||
| throw new SchemaNotFoundException(namespace); | |||
| } | |||
| catch (RESTException e) { | |||
| throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to load namespace metadata for: '%s'".formatted(namespace), e); | |||
There was a problem hiding this comment.
Failed to load metadata for namespace '%s'
| })); | ||
| } | ||
| catch (RESTException e) { | ||
| throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to create namespace: '%s'".formatted(namespace), e); |
| restSessionCatalog.registerTable(convert(session), tableIdentifier, tableMetadata.metadataFileLocation()); | ||
| } | ||
| catch (RESTException e) { | ||
| throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to register table: '%s'".formatted(tableName.getTableName()), e); |
| } | ||
| } | ||
| catch (RESTException e) { | ||
| throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to unregister table: '%s'".formatted(tableName.getTableName()), e); |
| throw new TrinoException(ICEBERG_CATALOG_ERROR, format("Failed to drop table: %s", schemaTableName)); | ||
| try { | ||
| if (!restSessionCatalog.purgeTable(convert(session), toRemoteTable(session, schemaTableName, true))) { | ||
| throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to drop table: '%s'".formatted(schemaTableName)); |
| throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to drop table: '%s'".formatted(schemaTableName)); | ||
| } | ||
| } | ||
| catch (RESTException e) { |
| baseTable = (BaseTable) restSessionCatalog.loadTable(convert(session), toRemoteObject(session, schemaTableName)); | ||
| } | ||
| catch (RESTException e) { | ||
| throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to load table: '%s'".formatted(schemaTableName.getTableName()), e); |
|
Drop these : as these are redundant. "Failed to drop table 'test'" looks better than "Failed to drop table: 'test'" |
mayankvadariya
force-pushed
the
mayank/rest-catalog-exception
branch
from
593f5c6 to
e79875b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Failing stack trace
"org.apache.iceberg.rest.ErrorHandlers$OAuthErrorHandler.accept(ErrorHandlers.java:247)",
"org.apache.iceberg.rest.ErrorHandlers$OAuthErrorHandler.accept(ErrorHandlers.java:228)",
"org.apache.iceberg.rest.HTTPClient.throwFailure(HTTPClient.java:211)",
"org.apache.iceberg.rest.HTTPClient.execute(HTTPClient.java:323)",
"org.apache.iceberg.rest.HTTPClient.execute(HTTPClient.java:262)",
"org.apache.iceberg.rest.HTTPClient.postForm(HTTPClient.java:409)",
"org.apache.iceberg.rest.auth.OAuth2Util.fetchToken(OAuth2Util.java:264)",
"org.apache.iceberg.rest.auth.OAuth2Util$AuthSession.fromCredential(OAuth2Util.java:693)",
"org.apache.iceberg.rest.RESTSessionCatalog.lambda$newSession$6(RESTSessionCatalog.java:1076)",
"org.apache.iceberg.rest.RESTSessionCatalog.lambda$session$1(RESTSessionCatalog.java:342)",
"com.github.benmanes.caffeine.cache.BoundedLocalCache.lambda$doComputeIfAbsent$14(BoundedLocalCache.java:2688)",
"java.base/java.util.concurrent.ConcurrentHashMap.compute(ConcurrentHashMap.java:1921)",
"com.github.benmanes.caffeine.cache.BoundedLocalCache.doComputeIfAbsent(BoundedLocalCache.java:2686)",
"com.github.benmanes.caffeine.cache.BoundedLocalCache.computeIfAbsent(BoundedLocalCache.java:2669)",
"com.github.benmanes.caffeine.cache.LocalCache.computeIfAbsent(LocalCache.java:112)",
"com.github.benmanes.caffeine.cache.LocalManualCache.get(LocalManualCache.java:62)",
"org.apache.iceberg.rest.RESTSessionCatalog.session(RESTSessionCatalog.java:336)",
"org.apache.iceberg.rest.RESTSessionCatalog.headers(RESTSessionCatalog.java:352)",
"org.apache.iceberg.rest.RESTSessionCatalog.listNamespaces(RESTSessionCatalog.java:634)",
"org.apache.iceberg.catalog.SessionCatalog.listNamespaces(SessionCatalog.java:276)",
"io.trino.plugin.iceberg.catalog.rest.TrinoRestCatalog.listNamespaces(TrinoRestCatalog.java:190)",
"io.trino.plugin.iceberg.IcebergMetadata.listSchemaNames(IcebergMetadata.java:486)",
Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.