superdiaodiao commented on PR #17464: URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5582593376
All applied in c575464 — class and both test methods are package-private, and the table/metadata locations are now `TABLE_LOCATION` / `METADATA_LOCATION` constants. I left `SCHEMA` as its own constant rather than inlining it; happy to fold it in if you prefer. ``` $ ./gradlew :iceberg-core:test --tests "org.apache.iceberg.TestBaseMetastoreTableOperations" tests="2" skipped="0" failures="0" errors="0" ``` > Is it enough to do it in the Hive codepath? What about other catalogs? I checked, and Hive is the only view implementation that needs it. `checkCurrentMetadataLocation` exists in exactly two places: - `BaseMetastoreTableOperations` — the **table** path, shared by every metastore-backed catalog. That is the other half of this PR, so all catalogs are covered there. - `HiveViewOperations` — the **view** path, and the only one. The other view implementations never reach this code: `NessieViewOperations`, `JdbcViewOperations` and `RESTViewOperations` have no commit-status check at all — no `CommitStatus`, no `UNKNOWN` retry path, nothing that calls `refresh()` to compare metadata locations. `BaseViewOperations` does not define one either. `HiveViewOperations` is the only view operations class that implements the "commit failed, but did it actually land?" check, which is precisely the path that NPEs. So the two guards in this PR cover both places where the pattern exists. If a future catalog adds view-side commit-status checking, it would need the same guard — but there is nothing to fix today. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
