laserninja commented on PR #10800: URL: https://github.com/apache/gravitino/pull/10800#issuecomment-4725810950
## Verification: All Requested Changes ✓ Excellent follow-up! The latest revision addresses all the HIGH and MEDIUM issues raised in the previous review: ### ✅ Issues Fixed **[HIGH] Bypasses dispatcher layer** - Credential vending now routes through `IcebergTableOperationExecutor.planTableScan()` - Calls `getCredentialPrivilege()` and passes parameters to `CatalogWrapperForREST.planTableScan(...)` - `IcebergCatalogWrapperManager` field removed from `IcebergTableOperations` - Events and authorization hooks will now fire correctly **[HIGH] Broad `catch (Exception e)`** - Narrowed to only catch `ServiceUnavailableException` in `injectScanCredentials()` - Matches the pattern in `getTableCredentials()` - Auth failures, illegal args, and other errors now propagate properly **[HIGH] Redundant `loadTable` call** - `injectScanCredentials()` receives the `Table` already loaded for scan planning - No additional `loadTable` invocation - Credential generation uses `getCredentialFromTable()` which extracts location/properties from the Table **[MEDIUM] JSON-level merge fragility** - Replaced ad-hoc `ObjectNode`/`ArrayNode` JSON construction with `PlanTableScanResponse.builder()...withCredentials(...)` - TODO comment notes future cleanup when Iceberg 1.11.0+ is adopted - Much cleaner and more maintainable **[MEDIUM] Test coverage for prefix & refresh properties** - Tests now assert `prefix` field is present and equals table location - `IcebergRESTUtils.buildRefreshProps()` and `toRESTCredential()` ensure refresh endpoints are included - Consistent with `getTableCredentials` pattern ### 📝 Minor Items **Duplicate Javadoc on no-arg `planTableScan` overload** The original method's full Javadoc still appears above the new `/** Plan table scan without credential vending. */` comment, resulting in two stacked `/** */` blocks. Consider removing the old one since it now applies only to the 4-arg overload (which has its own comprehensive Javadoc). **FederatedCatalogWrapper doesn't override `planTableScan`** `FederatedCatalogWrapper` still doesn't override the new 4-arg `planTableScan()` method. When federated catalogs call this, they'll use the local credential manager instead of proxying to the remote catalog. However: - The graceful fallback (`ServiceUnavailableException` → no credentials) prevents breakage - This could be addressed in a follow-up PR if federated credential vending on scans becomes important ### 🎯 Overall Assessment **Ready to merge** with one minor cleanup (duplicate Javadoc). The architectural improvements are solid: - Proper dispatcher routing ✓ - Credential reuse (no redundant loads) ✓ - Scoped exception handling ✓ - Consistent with existing credential vending patterns ✓ - Comprehensive test coverage ✓ -- 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]
