Copilot commented on code in PR #10536:
URL: https://github.com/apache/gravitino/pull/10536#discussion_r2985276613
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java:
##########
@@ -311,13 +313,23 @@ public Response loadTable(
TableIdentifier tableIdentifier = TableIdentifier.of(icebergNS,
tableName);
IcebergRequestContext context =
new IcebergRequestContext(httpServletRequest(), catalogName,
isCredentialVending);
+
+ // Fast path: if client sent If-None-Match, try to resolve ETag
without full table load
+ if (ifNoneMatch != null && !ifNoneMatch.isEmpty()) {
+ String metadataLocation =
+ tableOperationDispatcher.getTableMetadataLocation(context,
tableIdentifier);
+ if (metadataLocation != null) {
+ EntityTag etag = generateETag(metadataLocation, snapshots);
+ if (etag != null && etagMatches(ifNoneMatch, etag)) {
+ return Response.notModified(etag).build();
+ }
+ }
+ }
+
LoadTableResponse loadTableResponse =
tableOperationDispatcher.loadTable(context, tableIdentifier);
EntityTag etag =
generateETag(loadTableResponse.tableMetadata().metadataFileLocation(),
snapshots);
Review Comment:
`loadTable` no longer returns `304 Not Modified` for catalogs that *don't*
support `getTableMetadataLocation(...)`. If `metadataLocation` is null (or the
client didn't send `If-None-Match`), the code always proceeds to
`loadTable(...)` and then returns `200` even when `ifNoneMatch` matches the
computed ETag. This is a behavior regression from the previous logic.
Suggestion: keep the new fast-path, but after `loadTable(...)` compute the
ETag and still perform the `etagMatches(ifNoneMatch, etag)` check to return
`Response.notModified(etag)` when appropriate (fallback path).
```suggestion
generateETag(loadTableResponse.tableMetadata().metadataFileLocation(),
snapshots);
if (etag != null && ifNoneMatch != null &&
!ifNoneMatch.isEmpty()) {
if (etagMatches(ifNoneMatch, etag)) {
return Response.notModified(etag).build();
}
}
```
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java:
##########
@@ -204,6 +204,22 @@ public LoadTableResponse loadTable(TableIdentifier
tableIdentifier) {
return loadTableResponse;
}
+ /**
+ * Retrieves the metadata file location for the specified table without
loading full table
+ * metadata. This is an optional fast path for catalogs that implement {@link
+ * SupportsMetadataLocation}.
+ *
+ * @param tableIdentifier the table identifier
+ * @return the metadata file location, or null if the catalog doesn't
support this operation
+ */
+ @javax.annotation.Nullable
+ public String getTableMetadataLocation(TableIdentifier tableIdentifier) {
+ if (catalog instanceof SupportsMetadataLocation) {
+ return ((SupportsMetadataLocation)
catalog).metadataLocation(tableIdentifier);
+ }
+ return null;
Review Comment:
This method uses a fully-qualified `@javax.annotation.Nullable` annotation.
Elsewhere in this PR (e.g., `IcebergTableOperationDispatcher`)
`javax.annotation.Nullable` is imported and used as `@Nullable`, and the
project guidelines prefer standard imports over FQNs.
Suggestion: add `import javax.annotation.Nullable;` and switch the
annotation to `@Nullable` for consistency and readability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
