This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 9c03883a86 [#9736][followup] feat(iceberg-rest): Add ETag header to
registerTable response (#10641)
9c03883a86 is described below
commit 9c03883a8649299432729de253c120862daf6721
Author: akshay thorat <[email protected]>
AuthorDate: Mon Apr 6 00:54:43 2026 -0700
[#9736][followup] feat(iceberg-rest): Add ETag header to registerTable
response (#10641)
### What changes were proposed in this pull request?
Add ETag header to the `registerTable` response for consistency with
`createTable` and `updateTable`, which already include ETags. This was
identified by @roryqi in [#10536
(comment)](https://github.com/apache/gravitino/pull/10536#issuecomment-4168013863).
### Why are the changes needed?
The `registerTable` endpoint returns a `LoadTableResponse` but did not
include an ETag header, unlike `createTable` and `updateTable`. This
meant clients could not reuse ETags from `registerTable` for subsequent
conditional `loadTable` requests (`If-None-Match`), breaking ETag
consistency across all endpoints that return `LoadTableResponse`.
### Does this PR introduce _any_ user-facing change?
No user-facing API changes. The `registerTable` response now includes an
`ETag` header when the table metadata has a metadata file location.
### How was this patch tested?
- Added `testRegisterTableReturnsETag` test verifying ETag presence in
`registerTable` response.
- Updated test mock (`CatalogWrapperForTest`) to return `TableMetadata`
with a `metadataFileLocation` so ETag can be generated.
- All existing ETag and namespace tests pass.
---
.../iceberg/service/IcebergRESTUtils.java | 106 +++++++++++++++++++++
.../service/rest/IcebergNamespaceOperations.java | 2 +-
.../service/rest/IcebergTableOperations.java | 97 +++----------------
.../service/rest/CatalogWrapperForTest.java | 6 +-
.../service/rest/IcebergNamespaceTestBase.java | 2 +-
.../rest/TestIcebergNamespaceOperations.java | 23 +++++
6 files changed, 151 insertions(+), 85 deletions(-)
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
index 78ffd7e801..42ccd94ee4 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
@@ -21,6 +21,9 @@ package org.apache.gravitino.iceberg.service;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.base.Preconditions;
import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
import java.time.Instant;
import java.time.LocalDateTime;
import java.time.ZoneId;
@@ -28,8 +31,10 @@ import java.util.Arrays;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.Map;
+import java.util.Optional;
import java.util.stream.Stream;
import javax.servlet.http.HttpServletRequest;
+import javax.ws.rs.core.EntityTag;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import javax.ws.rs.core.Response.Status;
@@ -39,15 +44,116 @@ import
org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerConte
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.rest.responses.ErrorResponse;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class IcebergRESTUtils {
+ private static final Logger LOG =
LoggerFactory.getLogger(IcebergRESTUtils.class);
+
+ public static final String SNAPSHOT_ALL = "all";
+
+ public static final String SNAPSHOT_REFS = "refs";
+
+ /** Snapshot modes for the Iceberg loadTable endpoint. */
+ public enum SnapshotMode {
+ ALL(SNAPSHOT_ALL),
+ REFS(SNAPSHOT_REFS);
+
+ private final String value;
+
+ SnapshotMode(String value) {
+ this.value = value;
+ }
+
+ public String getValue() {
+ return value;
+ }
+ }
+
private IcebergRESTUtils() {}
public static <T> Response ok(T t) {
return
Response.status(Response.Status.OK).entity(t).type(MediaType.APPLICATION_JSON).build();
}
+ /**
+ * Builds an OK response with the ETag header derived from the table
metadata location. Uses the
+ * default snapshots value to ensure ETags from create/update/register are
consistent with the
+ * default loadTable endpoint.
+ *
+ * @param loadTableResponse the table response to include in the body
+ * @return a Response with ETag header set
+ */
+ public static Response buildResponseWithETag(LoadTableResponse
loadTableResponse) {
+ Optional<EntityTag> etag =
+ generateETag(
+ loadTableResponse.tableMetadata().metadataFileLocation(),
SnapshotMode.ALL.getValue());
+ return buildResponseWithETag(loadTableResponse, etag);
+ }
+
+ /**
+ * Builds an OK response with the given ETag header.
+ *
+ * @param loadTableResponse the table response to include in the body
+ * @param etag the pre-computed ETag
+ * @return a Response with ETag header set if etag is present
+ */
+ public static Response buildResponseWithETag(
+ LoadTableResponse loadTableResponse, Optional<EntityTag> etag) {
+ Response.ResponseBuilder responseBuilder =
+ Response.ok(loadTableResponse, MediaType.APPLICATION_JSON_TYPE);
+ etag.ifPresent(responseBuilder::tag);
+ return responseBuilder.build();
+ }
+
+ /**
+ * Generates an ETag based on the table metadata file location. The ETag is
a SHA-256 hash of the
+ * metadata location, which changes whenever the table metadata is updated.
Uses the default
+ * snapshots value to ensure consistency.
+ *
+ * @param metadataLocation the metadata file location
+ * @return the generated ETag
+ */
+ public static Optional<EntityTag> generateETag(String metadataLocation) {
+ return generateETag(metadataLocation, SnapshotMode.ALL.getValue());
+ }
+
+ /**
+ * Generates an ETag based on the table metadata file location and snapshot
mode. The ETag is a
+ * SHA-256 hash that incorporates both the metadata location and the
snapshots parameter, ensuring
+ * distinct ETags for different representations of the same table version
(e.g., snapshots=all vs
+ * snapshots=refs).
+ *
+ * @param metadataLocation the metadata file location
+ * @param snapshots the snapshots query parameter value (e.g., "all", "refs")
+ * @return the generated ETag
+ */
+ public static Optional<EntityTag> generateETag(String metadataLocation,
String snapshots) {
+ if (StringUtils.isBlank(metadataLocation)) {
+ return Optional.empty();
+ }
+ try {
+ MessageDigest digest = MessageDigest.getInstance("SHA-256");
+ digest.update(metadataLocation.getBytes(StandardCharsets.UTF_8));
+ digest.update(snapshots.getBytes(StandardCharsets.UTF_8));
+ byte[] hash = digest.digest();
+ StringBuilder hexString = new StringBuilder();
+ for (byte b : hash) {
+ String hex = Integer.toHexString(0xff & b);
+ if (hex.length() == 1) {
+ hexString.append('0');
+ }
+ hexString.append(hex);
+ }
+ return Optional.of(new EntityTag(hexString.toString()));
+ } catch (NoSuchAlgorithmException e) {
+ LOG.warn("Failed to generate ETag for metadata location: {}",
metadataLocation, e);
+ return Optional.empty();
+ }
+ }
+
public static Response okWithoutContent() {
return Response.status(Response.Status.OK).build();
}
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceOperations.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceOperations.java
index 1c669b86b6..ff09be9828 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceOperations.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceOperations.java
@@ -317,7 +317,7 @@ public class IcebergNamespaceOperations {
LoadTableResponse loadTableResponse =
namespaceOperationDispatcher.registerTable(
context, icebergNS, registerTableRequest);
- return IcebergRESTUtils.ok(loadTableResponse);
+ return IcebergRESTUtils.buildResponseWithETag(loadTableResponse);
});
} catch (Exception e) {
return IcebergExceptionMapper.toRESTResponse(e);
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
index 7754f1037c..18d5adbdde 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java
@@ -23,9 +23,6 @@ import com.codahale.metrics.annotation.Timed;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.annotations.VisibleForTesting;
-import java.nio.charset.StandardCharsets;
-import java.security.MessageDigest;
-import java.security.NoSuchAlgorithmException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
@@ -98,9 +95,6 @@ public class IcebergTableOperations {
@VisibleForTesting public static final String IF_NONE_MATCH =
"If-None-Match";
- @VisibleForTesting static final String DEFAULT_SNAPSHOTS = "all";
- @VisibleForTesting static final String SNAPSHOTS_REFS = "refs";
-
private IcebergMetricsManager icebergMetricsManager;
private ObjectMapper icebergObjectMapper;
@@ -297,7 +291,7 @@ public class IcebergTableOperations {
String namespace,
@IcebergAuthorizationMetadata(type = RequestType.LOAD_TABLE) @Encoded()
@PathParam("table")
String table,
- @DefaultValue(DEFAULT_SNAPSHOTS) @QueryParam("snapshots") String
snapshots,
+ @DefaultValue(IcebergRESTUtils.SNAPSHOT_ALL) @QueryParam("snapshots")
String snapshots,
@HeaderParam(X_ICEBERG_ACCESS_DELEGATION) String accessDelegation,
@HeaderParam(IF_NONE_MATCH) String ifNoneMatch) {
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
@@ -325,21 +319,21 @@ public class IcebergTableOperations {
Optional<String> metadataLocation =
tableOperationDispatcher.getTableMetadataLocation(context,
tableIdentifier);
if (metadataLocation.isPresent()) {
- EntityTag etag = generateETag(metadataLocation.get(),
snapshots);
- if (etag != null && etagMatches(ifNoneMatch, etag)) {
- return Response.notModified(etag).build();
+ Optional<EntityTag> etag =
generateETag(metadataLocation.get(), snapshots);
+ if (etag.isPresent() && etagMatches(ifNoneMatch, etag.get())) {
+ return Response.notModified(etag.get()).build();
}
}
}
LoadTableResponse loadTableResponse =
tableOperationDispatcher.loadTable(context, tableIdentifier);
- EntityTag etag =
+ Optional<EntityTag> etag =
generateETag(loadTableResponse.tableMetadata().metadataFileLocation(),
snapshots);
- if (etag != null && etagMatches(ifNoneMatch, etag)) {
- return Response.notModified(etag).build();
+ if (etag.isPresent() && etagMatches(ifNoneMatch, etag.get())) {
+ return Response.notModified(etag.get()).build();
}
- if (SNAPSHOTS_REFS.equals(snapshots)) {
+ if
(IcebergRESTUtils.SnapshotMode.REFS.getValue().equals(snapshots)) {
loadTableResponse = filterSnapshotsByRefs(loadTableResponse);
}
return buildResponseWithETag(loadTableResponse, etag);
@@ -571,84 +565,23 @@ public class IcebergTableOperations {
.build();
}
- /**
- * Builds an OK response with the ETag header derived from the table
metadata location. Uses the
- * default snapshots value to ensure ETags from create/update are consistent
with the default
- * loadTable endpoint.
- *
- * @param loadTableResponse the table response to include in the body
- * @return a Response with ETag header set
- */
private static Response buildResponseWithETag(LoadTableResponse
loadTableResponse) {
- EntityTag etag =
- generateETag(loadTableResponse.tableMetadata().metadataFileLocation(),
DEFAULT_SNAPSHOTS);
- return buildResponseWithETag(loadTableResponse, etag);
+ return IcebergRESTUtils.buildResponseWithETag(loadTableResponse);
}
- /**
- * Builds an OK response with the given ETag header.
- *
- * @param loadTableResponse the table response to include in the body
- * @param etag the pre-computed ETag, may be null
- * @return a Response with ETag header set if etag is non-null
- */
private static Response buildResponseWithETag(
- LoadTableResponse loadTableResponse, EntityTag etag) {
- Response.ResponseBuilder responseBuilder =
- Response.ok(loadTableResponse, MediaType.APPLICATION_JSON_TYPE);
- if (etag != null) {
- responseBuilder.tag(etag);
- }
- return responseBuilder.build();
+ LoadTableResponse loadTableResponse, Optional<EntityTag> etag) {
+ return IcebergRESTUtils.buildResponseWithETag(loadTableResponse, etag);
}
- /**
- * Generates an ETag based on the table metadata file location. The ETag is
a SHA-256 hash of the
- * metadata location, which changes whenever the table metadata is updated.
- *
- * @param metadataLocation the metadata file location
- * @return the generated ETag, or null if generation fails
- */
@VisibleForTesting
- static EntityTag generateETag(String metadataLocation) {
- return generateETag(metadataLocation, null);
+ static Optional<EntityTag> generateETag(String metadataLocation) {
+ return IcebergRESTUtils.generateETag(metadataLocation);
}
- /**
- * Generates an ETag based on the table metadata file location and snapshot
mode. The ETag is a
- * SHA-256 hash that incorporates both the metadata location and the
snapshots parameter, ensuring
- * distinct ETags for different representations of the same table version
(e.g., snapshots=all vs
- * snapshots=refs).
- *
- * @param metadataLocation the metadata file location
- * @param snapshots the snapshots query parameter value (e.g., "all",
"refs"), may be null
- * @return the generated ETag, or null if generation fails
- */
@VisibleForTesting
- static EntityTag generateETag(String metadataLocation, String snapshots) {
- if (metadataLocation == null) {
- return null;
- }
- try {
- MessageDigest digest = MessageDigest.getInstance("SHA-256");
- digest.update(metadataLocation.getBytes(StandardCharsets.UTF_8));
- if (snapshots != null) {
- digest.update(snapshots.getBytes(StandardCharsets.UTF_8));
- }
- byte[] hash = digest.digest();
- StringBuilder hexString = new StringBuilder();
- for (byte b : hash) {
- String hex = Integer.toHexString(0xff & b);
- if (hex.length() == 1) {
- hexString.append('0');
- }
- hexString.append(hex);
- }
- return new EntityTag(hexString.toString());
- } catch (NoSuchAlgorithmException e) {
- LOG.warn("Failed to generate ETag for metadata location: {}",
metadataLocation, e);
- return null;
- }
+ static Optional<EntityTag> generateETag(String metadataLocation, String
snapshots) {
+ return IcebergRESTUtils.generateETag(metadataLocation, snapshots);
}
/**
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/CatalogWrapperForTest.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/CatalogWrapperForTest.java
index 89150a02c8..94c29e5de1 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/CatalogWrapperForTest.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/CatalogWrapperForTest.java
@@ -31,6 +31,7 @@ import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
import org.apache.iceberg.TableMetadata;
+import org.apache.iceberg.TableMetadataParser;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.AlreadyExistsException;
@@ -66,9 +67,12 @@ public class CatalogWrapperForTest extends
CatalogWrapperForREST {
}
Schema mockSchema = new Schema(NestedField.of(1, false, "foo_string",
StringType.get()));
- TableMetadata tableMetadata =
+ TableMetadata baseMetadata =
TableMetadata.newTableMetadata(
mockSchema, PartitionSpec.unpartitioned(), "/mock",
ImmutableMap.of());
+ String json = TableMetadataParser.toJson(baseMetadata);
+ TableMetadata tableMetadata =
+ TableMetadataParser.fromJson("/mock/metadata/v1.metadata.json", json);
LoadTableResponse loadTableResponse =
LoadTableResponse.builder()
.withTableMetadata(tableMetadata)
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceTestBase.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceTestBase.java
index 1362bb2461..bd749821fa 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceTestBase.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/IcebergNamespaceTestBase.java
@@ -52,7 +52,7 @@ public class IcebergNamespaceTestBase extends IcebergTestBase
{
.post(Entity.entity(request, MediaType.APPLICATION_JSON_TYPE));
}
- private Response doRegisterTable(String tableName, Namespace ns) {
+ protected Response doRegisterTable(String tableName, Namespace ns) {
RegisterTableRequest request =
ImmutableRegisterTableRequest.builder().name(tableName).metadataLocation("mock").build();
return getNamespaceClientBuilder(Optional.of(ns), Optional.of("register"),
Optional.empty())
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java
index 1f07808687..2bd62af968 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java
@@ -22,6 +22,9 @@ import java.util.Arrays;
import java.util.Optional;
import javax.servlet.http.HttpServletRequest;
import javax.ws.rs.core.Application;
+import javax.ws.rs.core.EntityTag;
+import javax.ws.rs.core.Response;
+import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
import org.apache.gravitino.listener.api.event.Event;
import org.apache.gravitino.listener.api.event.IcebergCreateNamespaceEvent;
import
org.apache.gravitino.listener.api.event.IcebergCreateNamespaceFailureEvent;
@@ -189,6 +192,26 @@ public class TestIcebergNamespaceOperations extends
IcebergNamespaceTestBase {
verifyRegisterTableSucc("register_foo2", Namespace.of("register_ns_2",
"a"));
}
+ @Test
+ void testRegisterTableReturnsETag() {
+ Namespace ns = Namespace.of("register_etag_ns");
+ Response response = doRegisterTable("register_etag_foo1", ns);
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
response.getStatus());
+ String etag = response.getHeaderString("ETag");
+ Assertions.assertNotNull(etag, "ETag header should be present in register
table response");
+ Assertions.assertFalse(etag.isEmpty(), "ETag header should not be empty");
+
+ // Verify the ETag value matches the expected SHA-256 hash of the known
mock metadata location
+ Optional<EntityTag> expectedETag =
+ IcebergRESTUtils.generateETag(
+ "/mock/metadata/v1.metadata.json",
IcebergRESTUtils.SnapshotMode.ALL.getValue());
+ Assertions.assertTrue(expectedETag.isPresent(), "Expected ETag should be
generated");
+ Assertions.assertEquals(
+ "\"" + expectedETag.get().getValue() + "\"",
+ etag,
+ "ETag should match SHA-256 hash of mock metadata location with default
snapshots");
+ }
+
@ParameterizedTest
@ValueSource(strings = {"", IcebergRestTestUtil.PREFIX})
void testListNamespace(String prefix) {
