laserninja commented on PR #10536: URL: https://github.com/apache/gravitino/pull/10536#issuecomment-4127741283
@FANNG1 Thanks for the suggestion! I considered this approach but went with a separate `getTableMetadataLocation` for a few reasons: 1. **Separation of concerns**: ETag/conditional-load is an HTTP caching concern. Keeping it in the REST layer (`IcebergTableOperations`) avoids leaking HTTP semantics into the catalog abstraction. 2. **Minimal contract change**: `loadTable` has a well-defined return type (`LoadTableResponse`). Adding conditional-load support would require changing the return type to a wrapper/union type, which touches many more callers. 3. **`SupportsMetadataLocation` already exists**: We're exposing an existing catalog capability, not inventing a new abstraction. The interface is already implemented by JDBC/Hive/Memory catalogs. 4. **The fast path is optional**: `getTableMetadataLocation` returns `Optional.empty()` for unsupported catalogs, and the full `loadTable` fallback always works. That said, if you feel strongly about encapsulating it within `loadTable`, I'm happy to refactor. Would appreciate your thoughts on how you'd see the return type working (e.g., `Optional<LoadTableResponse>`, or a new `ConditionalLoadResult` wrapper)? -- 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]
