Skip to content

Commit 7e82f2c

Browse files
dennishuodennishuo
authored
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
1 parent b50e594 commit 7e82f2c

File tree

3 files changed

+74
-34
lines changed

3 files changed

+74
-34
lines changed

‎service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java‎

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,31 @@ private static Namespace decodeNamespace(String namespace) {
235235
return RESTUtil.decodeNamespace(URLEncoder.encode(namespace, Charset.defaultCharset()));
236236
}
237237

238+
/**
239+
* For situations where we typically expect a metadataLocation to be present in the response and
240+
* so expect to insert an etag header, this helper gracefully falls back to omitting the header if
241+
* unable to get metadata location and logs a warning.
242+
*/
243+
private Response.ResponseBuilder tryInsertETagHeader(
244+
Response.ResponseBuilder builder,
245+
LoadTableResponse response,
246+
String namespace,
247+
String tableName) {
248+
if (response.metadataLocation() != null) {
249+
builder =
250+
builder.header(
251+
HttpHeaders.ETAG,
252+
IcebergHttpUtil.generateETagForMetadataFileLocation(response.metadataLocation()));
253+
} else {
254+
LOGGER
255+
.atWarn()
256+
.addKeyValue("namespace", namespace)
257+
.addKeyValue("tableName", tableName)
258+
.log("Response has null metadataLocation; omitting etag");
259+
}
260+
return builder;
261+
}
262+
238263
@Override
239264
public Response namespaceExists(
240265
String prefix, String namespace, RealmContext realmContext, SecurityContext securityContext) {
@@ -312,20 +337,14 @@ public Response createTable(
312337
}
313338
} else if (delegationModes.isEmpty()) {
314339
LoadTableResponse response = catalog.createTableDirect(ns, createTableRequest);
315-
return Response.ok(response)
316-
.header(
317-
HttpHeaders.ETAG,
318-
IcebergHttpUtil.generateETagForMetadataFileLocation(
319-
response.metadataLocation()))
340+
return tryInsertETagHeader(
341+
Response.ok(response), response, namespace, createTableRequest.name())
320342
.build();
321343
} else {
322344
LoadTableResponse response =
323345
catalog.createTableDirectWithWriteDelegation(ns, createTableRequest);
324-
return Response.ok(response)
325-
.header(
326-
HttpHeaders.ETAG,
327-
IcebergHttpUtil.generateETagForMetadataFileLocation(
328-
response.metadataLocation()))
346+
return tryInsertETagHeader(
347+
Response.ok(response), response, namespace, createTableRequest.name())
329348
.build();
330349
}
331350
});
@@ -384,11 +403,7 @@ public Response loadTable(
384403
.orElseThrow(() -> new WebApplicationException(Response.Status.NOT_MODIFIED));
385404
}
386405

387-
return Response.ok(response)
388-
.header(
389-
HttpHeaders.ETAG,
390-
IcebergHttpUtil.generateETagForMetadataFileLocation(response.metadataLocation()))
391-
.build();
406+
return tryInsertETagHeader(Response.ok(response), response, namespace, table).build();
392407
});
393408
}
394409

@@ -446,11 +461,8 @@ public Response registerTable(
446461
prefix,
447462
catalog -> {
448463
LoadTableResponse response = catalog.registerTable(ns, registerTableRequest);
449-
450-
return Response.ok(response)
451-
.header(
452-
HttpHeaders.ETAG,
453-
IcebergHttpUtil.generateETagForMetadataFileLocation(response.metadataLocation()))
464+
return tryInsertETagHeader(
465+
Response.ok(response), response, namespace, registerTableRequest.name())
454466
.build();
455467
});
456468
}

‎service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java‎

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -503,8 +503,7 @@ public boolean sendNotification(TableIdentifier identifier, NotificationRequest
503503
* @return the Polaris table entity for the table
504504
*/
505505
private IcebergTableLikeEntity getTableEntity(TableIdentifier tableIdentifier) {
506-
PolarisResolvedPathWrapper target =
507-
resolutionManifest.getPassthroughResolvedPath(tableIdentifier);
506+
PolarisResolvedPathWrapper target = resolutionManifest.getResolvedPath(tableIdentifier);
508507

509508
return IcebergTableLikeEntity.of(target.getRawLeafEntity());
510509
}
@@ -529,12 +528,24 @@ public Optional<LoadTableResponse> loadTableIfStale(
529528
authorizeBasicTableLikeOperationOrThrow(
530529
op, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
531530

532-
IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier);
533-
String tableEntityTag =
534-
IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation());
535-
536-
if (ifNoneMatch != null && ifNoneMatch.anyMatch(tableEntityTag)) {
537-
return Optional.empty();
531+
if (ifNoneMatch != null) {
532+
// Perform freshness-aware table loading if caller specified ifNoneMatch.
533+
IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier);
534+
if (tableEntity == null || tableEntity.getMetadataLocation() == null) {
535+
LOGGER
536+
.atWarn()
537+
.addKeyValue("tableIdentifier", tableIdentifier)
538+
.addKeyValue("tableEntity", tableEntity)
539+
.log("Failed to getMetadataLocation to generate ETag when loading table");
540+
} else {
541+
// TODO: Refactor null-checking into the helper method once we create a more canonical
542+
// interface for associate etags with entities.
543+
String tableEntityTag =
544+
IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation());
545+
if (ifNoneMatch.anyMatch(tableEntityTag)) {
546+
return Optional.empty();
547+
}
548+
}
538549
}
539550

540551
return Optional.of(CatalogHandlers.loadTable(baseCatalog, tableIdentifier));
@@ -601,12 +612,24 @@ public Optional<LoadTableResponse> loadTableWithAccessDelegationIfStale(
601612
FeatureConfiguration.ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING.catalogConfig());
602613
}
603614

604-
IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier);
605-
String tableETag =
606-
IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation());
607-
608-
if (ifNoneMatch != null && ifNoneMatch.anyMatch(tableETag)) {
609-
return Optional.empty();
615+
if (ifNoneMatch != null) {
616+
// Perform freshness-aware table loading if caller specified ifNoneMatch.
617+
IcebergTableLikeEntity tableEntity = getTableEntity(tableIdentifier);
618+
if (tableEntity == null || tableEntity.getMetadataLocation() == null) {
619+
LOGGER
620+
.atWarn()
621+
.addKeyValue("tableIdentifier", tableIdentifier)
622+
.addKeyValue("tableEntity", tableEntity)
623+
.log("Failed to getMetadataLocation to generate ETag when loading table");
624+
} else {
625+
// TODO: Refactor null-checking into the helper method once we create a more canonical
626+
// interface for associate etags with entities.
627+
String tableETag =
628+
IcebergHttpUtil.generateETagForMetadataFileLocation(tableEntity.getMetadataLocation());
629+
if (ifNoneMatch.anyMatch(tableETag)) {
630+
return Optional.empty();
631+
}
632+
}
610633
}
611634

612635
// TODO: Find a way for the configuration or caller to better express whether to fail or omit

‎service/common/src/main/java/org/apache/polaris/service/http/IcebergHttpUtil.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,11 @@ private IcebergHttpUtil() {}
3232
* @return the generated ETag
3333
*/
3434
public static String generateETagForMetadataFileLocation(String metadataFileLocation) {
35+
if (metadataFileLocation == null) {
36+
// Throw a more appropriate exception than letting DigestUtils die randomly.
37+
throw new IllegalArgumentException("Unable to generate etag for null metadataFileLocation");
38+
}
39+
3540
// Use hash of metadata location since we don't want clients to use the ETag to try to extract
3641
// the metadata file location
3742
String hashedMetadataFileLocation = DigestUtils.sha256Hex(metadataFileLocation);

0 commit comments

Comments
 (0)