This is an automated email from the ASF dual-hosted git repository.
dhuo pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/polaris.git
The following commit(s) were added to refs/heads/main by this push:
new 7e82f2c87 Improve code-containment and efficiency of etag-aware
loading (#1296)
7e82f2c87 is described below
commit 7e82f2c87d57430f84c704ab92def05b8e2fefdf
Author: Dennis Huo <[email protected]>
AuthorDate: Wed Apr 2 22:15:49 2025 -0700
Improve code-containment and efficiency of etag-aware loading (#1296)
* Improve code-containment and efficiency of etag-aware loading
-Make the hash generation resilient against null metadataLocation
-Use getResolvedPath instead of getPassthroughResolvedPath to avoid
redundant persistence round-trip
-Only try to calculate the etag for comparison against ifNoneMatch if the
ifNoneMatch is actually provided
* Add strict null-checking at callsites to generateETag, disallow passing
null to generator
* Add TODO to refactor shared logic for etag generation
---
.../catalog/iceberg/IcebergCatalogAdapter.java | 52 +++++++++++++---------
.../catalog/iceberg/IcebergCatalogHandler.java | 51 +++++++++++++++------
.../polaris/service/http/IcebergHttpUtil.java | 5 +++
3 files changed, 74 insertions(+), 34 deletions(-)
diff --git
a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java
b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java
index 6db06cb01..ccb6b44f5 100644
---
a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java
+++
b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java
@@ -235,6 +235,31 @@ public class IcebergCatalogAdapter
return RESTUtil.decodeNamespace(URLEncoder.encode(namespace,
Charset.defaultCharset()));
}
+ /**
+ * For situations where we typically expect a metadataLocation to be present
in the response and
+ * so expect to insert an etag header, this helper gracefully falls back to
omitting the header if
+ * unable to get metadata location and logs a warning.
+ */
+ private Response.ResponseBuilder tryInsertETagHeader(
+ Response.ResponseBuilder builder,
+ LoadTableResponse response,
+ String namespace,
+ String tableName) {
+ if (response.metadataLocation() != null) {
+ builder =
+ builder.header(
+ HttpHeaders.ETAG,
+
IcebergHttpUtil.generateETagForMetadataFileLocation(response.metadataLocation()));
+ } else {
+ LOGGER
+ .atWarn()
+ .addKeyValue("namespace", namespace)
+ .addKeyValue("tableName", tableName)
+ .log("Response has null metadataLocation; omitting etag");
+ }
+ return builder;
+ }
+
@Override
public Response namespaceExists(
String prefix, String namespace, RealmContext realmContext,
SecurityContext securityContext) {
@@ -312,20 +337,14 @@ public class IcebergCatalogAdapter
}
} else if (delegationModes.isEmpty()) {
LoadTableResponse response = catalog.createTableDirect(ns,
createTableRequest);
- return Response.ok(response)
- .header(
- HttpHeaders.ETAG,
- IcebergHttpUtil.generateETagForMetadataFileLocation(
- response.metadataLocation()))
+ return tryInsertETagHeader(
+ Response.ok(response), response, namespace,
createTableRequest.name())
.build();
} else {
LoadTableResponse response =
catalog.createTableDirectWithWriteDelegation(ns,
createTableRequest);
- return Response.ok(response)
- .header(
- HttpHeaders.ETAG,
- IcebergHttpUtil.generateETagForMetadataFileLocation(
- response.metadataLocation()))
+ return tryInsertETagHeader(
+ Response.ok(response), response, namespace,
createTableRequest.name())
.build();
}
});
@@ -384,11 +403,7 @@ public class IcebergCatalogAdapter
.orElseThrow(() -> new
WebApplicationException(Response.Status.NOT_MODIFIED));
}
- return Response.ok(response)
- .header(
- HttpHeaders.ETAG,
-
IcebergHttpUtil.generateETagForMetadataFileLocation(response.metadataLocation()))
- .build();
+ return tryInsertETagHeader(Response.ok(response), response,
namespace, table).build();
});
}
@@ -446,11 +461,8 @@ public class IcebergCatalogAdapter
prefix,
catalog -> {
LoadTableResponse response = catalog.registerTable(ns,
registerTableRequest);
-
- return Response.ok(response)
- .header(
- HttpHeaders.ETAG,
-
IcebergHttpUtil.generateETagForMetadataFileLocation(response.metadataLocation()))
+ return tryInsertETagHeader(
+ Response.ok(response), response, namespace,
registerTableRequest.name())
.build();
});
}
diff --git
a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
index 78fecf479..b5e1a0edb 100644
---
a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
+++
b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
@@ -503,8 +503,7 @@ public class IcebergCatalogHandler extends CatalogHandler
implements AutoCloseab
* @return the Polaris table entity for the table
*/
private IcebergTableLikeEntity getTableEntity(TableIdentifier
tableIdentifier) {
- PolarisResolvedPathWrapper target =
- resolutionManifest.getPassthroughResolvedPath(tableIdentifier);
+ PolarisResolvedPathWrapper target =
resolutionManifest.getResolvedPath(tableIdentifier);
return IcebergTableLikeEntity.of(target.getRawLeafEntity());
}
@@ -529,12 +528,24 @@ public class IcebergCatalogHandler extends CatalogHandler
implements AutoCloseab
authorizeBasicTableLikeOperationOrThrow(
op, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
- IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier);
- String tableEntityTag =
-
IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation());
-
- if (ifNoneMatch != null && ifNoneMatch.anyMatch(tableEntityTag)) {
- return Optional.empty();
+ if (ifNoneMatch != null) {
+ // Perform freshness-aware table loading if caller specified ifNoneMatch.
+ IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier);
+ if (tableEntity == null || tableEntity.getMetadataLocation() == null) {
+ LOGGER
+ .atWarn()
+ .addKeyValue("tableIdentifier", tableIdentifier)
+ .addKeyValue("tableEntity", tableEntity)
+ .log("Failed to getMetadataLocation to generate ETag when loading
table");
+ } else {
+ // TODO: Refactor null-checking into the helper method once we create
a more canonical
+ // interface for associate etags with entities.
+ String tableEntityTag =
+
IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation());
+ if (ifNoneMatch.anyMatch(tableEntityTag)) {
+ return Optional.empty();
+ }
+ }
}
return Optional.of(CatalogHandlers.loadTable(baseCatalog,
tableIdentifier));
@@ -601,12 +612,24 @@ public class IcebergCatalogHandler extends CatalogHandler
implements AutoCloseab
FeatureConfiguration.ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING.catalogConfig());
}
- IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier);
- String tableETag =
-
IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation());
-
- if (ifNoneMatch != null && ifNoneMatch.anyMatch(tableETag)) {
- return Optional.empty();
+ if (ifNoneMatch != null) {
+ // Perform freshness-aware table loading if caller specified ifNoneMatch.
+ IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier);
+ if (tableEntity == null || tableEntity.getMetadataLocation() == null) {
+ LOGGER
+ .atWarn()
+ .addKeyValue("tableIdentifier", tableIdentifier)
+ .addKeyValue("tableEntity", tableEntity)
+ .log("Failed to getMetadataLocation to generate ETag when loading
table");
+ } else {
+ // TODO: Refactor null-checking into the helper method once we create
a more canonical
+ // interface for associate etags with entities.
+ String tableETag =
+
IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation());
+ if (ifNoneMatch.anyMatch(tableETag)) {
+ return Optional.empty();
+ }
+ }
}
// TODO: Find a way for the configuration or caller to better express
whether to fail or omit
diff --git
a/service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java
b/service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java
index 88c056aa7..2fd0445d9 100644
---
a/service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java
+++
b/service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java
@@ -32,6 +32,11 @@ public class IcebergHttpUtil {
* @return the generated ETag
*/
public static String generateETagForMetadataFileLocation(String
metadataFileLocation) {
+ if (metadataFileLocation == null) {
+ // Throw a more appropriate exception than letting DigestUtils die
randomly.
+ throw new IllegalArgumentException("Unable to generate etag for null
metadataFileLocation");
+ }
+
// Use hash of metadata location since we don't want clients to use the
ETag to try to extract
// the metadata file location
String hashedMetadataFileLocation =
DigestUtils.sha256Hex(metadataFileLocation);
