superdiaodiao opened a new pull request, #17464: URL: https://github.com/apache/iceberg/pull/17464
## Which issue does this PR close? - Closes #17462 ## What changes are included in this PR? When a CREATE TABLE commit fails with a non-specific exception (e.g. a Thrift socket timeout from HMS), `HiveTableOperations#doCommit` calls `checkCommitStatus(...)` to determine whether the commit landed. For a table that was never persisted, `doRefresh()` swallows `NoSuchObjectException` (expected for creates) and refreshes from a `null` metadata location, leaving `current()` as `null`. `checkCurrentMetadataLocation` then dereferences the null metadata and throws an NPE on every status-check attempt. The NPEs are suppressed (`suppressFailureWhenFinished`), the status stays `UNKNOWN`, and the user gets a `CommitStateUnknownException` recommending manual intervention - even though the outcome was knowable. This PR null-guards the commit status supplier in two places: - `BaseMetastoreTableOperations#checkCurrentMetadataLocation`: if `refresh()` yields no metadata, the table does not exist in the catalog, so the new metadata location cannot be current or in history - return `false`. - `HiveViewOperations#checkCurrentMetadataLocation`: same pattern and null-guard for the view counterpart. With the guard in place: - `checkCommitStatusStrict` now correctly resolves to `FAILURE` for failed create commits (previously: NPE -> suppressed -> `UNKNOWN`), so the concurrent-modification branch in `HiveTableOperations#doCommit` throws `CommitFailedException` instead of `CommitStateUnknownException`. - The relaxed `checkCommitStatus` cleanly resolves the supplier to `false` and maps it to `UNKNOWN` per its documented semantics (#12637), with no NPE spam in the logs. Whether failed creates should hard-fail in the relaxed path as well is a semantics question intentionally left out of this PR (see the review discussion on #6499). This revives the core part of #6499, which diagnosed and fixed the same problem in Dec 2022 but was closed by the stale bot without a decision. The code has since moved (the status check was refactored into `BaseMetastoreOperations` by #12637), so the guard now lives in `checkCurrentMetadataLocation`. ## Are these changes tested? Yes - a new core unit test, `TestBaseMetastoreTableOperations`, with a minimal `BaseMetastoreTableOperations` subclass whose `doRefresh()` mimics `HiveTableOperations` for a never-persisted table (missing table is not an error when no metadata location is known -> refresh from `null` -> current metadata stays `null`): - `strictStatusCheckIsFailureWhenTableWasNeverPersisted` - fails without the fix (the NPE is suppressed and the status stays `UNKNOWN`) and passes with it. - `statusCheckIsUnknownWhenTableWasNeverPersisted` - pins the relaxed-check semantics for the same scenario. Verified locally: the strict test fails on `main` without the fix and passes with it; `:iceberg-core:test` and `:iceberg-hive-metastore:test` both pass with the change. -- 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]
