krisnaru opened a new issue, #17462:
URL: https://github.com/apache/iceberg/issues/17462
### Apache Iceberg version
1.11.0 (latest release)
### Query engine
Spark
### Please describe the bug 🐞
checkCommitStatus throws NullPointerException when the failing commit is a
CREATE TABLE, which turns a knowable commit failure into a
CommitStateUnknownException telling the user to perform manual intervention
that isn't actually needed.
## What happens
When HiveTableOperations.doCommit fails with a non-specific exception
(e.g. a Thrift TTransportException / socket read timeout from HMS), it falls
into the generic catch (Throwable e) branch and calls checkCommitStatus(...) to
determine whether the commit landed. For a new table (base == null) that was
never persisted to the metastore, every status-check attempt NPEs:
ERROR BaseMetastoreTableOperations: Cannot check if commit to db.tbl
exists.
```
java.lang.NullPointerException
at
org.apache.iceberg.BaseMetastoreTableOperations.lambda$checkCommitStatus$4(BaseMetastoreTableOperations.java:359)
at
org.apache.iceberg.util.Tasks$Builder.runTaskWithRetry(Tasks.java:413)
at
org.apache.iceberg.util.Tasks$Builder.runSingleThreaded(Tasks.java:219)
at org.apache.iceberg.util.Tasks$Builder.run(Tasks.java:203)
at org.apache.iceberg.util.Tasks$Builder.run(Tasks.java:196)
at
org.apache.iceberg.BaseMetastoreTableOperations.checkCommitStatus(BaseMetastoreTableOperations.java:356)
at
org.apache.iceberg.hive.HiveTableOperations.doCommit(HiveTableOperations.java:303)
at
org.apache.iceberg.BaseMetastoreTableOperations.commit(BaseMetastoreTableOperations.java:141)
at
org.apache.iceberg.BaseMetastoreCatalog$BaseMetastoreCatalogTableBuilder.create(BaseMetastoreCatalog.java:199)
```
ERROR BaseMetastoreTableOperations: Cannot determine commit state to
db.tbl. Failed during checking 3 times. Treating commit state as unknown.
Because checkCommitStatus uses .suppressFailureWhenFinished(), the NPEs
are swallowed, the status stays UNKNOWN, and the user gets:
org.apache.iceberg.exceptions.CommitStateUnknownException:
java.net.SocketTimeoutException: Read timed out
Cannot determine whether the commit was successful or not, the underlying
data files may or may not be needed.
Manual intervention via the Remove Orphan Files Action can remove these
files when a connection to the Catalog can be re-established if the commit was
actually unsuccessful.
Please check to see whether or not your commit was successful before
retrying this commit. Retrying an already
successful operation will result in duplicate records or unintentional
modifications.
The commit status was in fact knowable — the table does not exist in HMS,
so the correct status is FAILURE.
Root cause
The line numbers above are from a 1.7.x build, but the code path is
unchanged on main (verified at 6b65d514cd):
1. checkCurrentMetadataLocation dereferences the result of refresh() with
no null check — BaseMetastoreTableOperations.java#L334-L336
(https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java#L334-L336):
private boolean checkCurrentMetadataLocation(String newMetadataLocation) {
TableMetadata metadata = refresh();
String currentMetadataFileLocation = metadata.metadataFileLocation();
// <-- NPE
...
}
2. refresh() returns null rather than throwing in this scenario.
HiveTableOperations.doRefresh() swallows NoSuchObjectException when
currentMetadataLocation() == null, which is exactly
the create case — HiveTableOperations.java#L191-L194
(https://github.com/apache/iceberg/blob/main/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java#L191-L194).
3. It then calls refreshFromMetadataLocation(null, ...). The guard is if
(!Objects.equal(currentMetadataLocation, newLocation)) — both are null, so the
body is skipped entirely, leaving
currentMetadata == null while setting shouldRefresh = false —
BaseMetastoreTableOperations.java#L189-L215
(https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java#L189-L215).
4. current() therefore returns the null currentMetadata instead of
re-refreshing — BaseMetastoreTableOperations.java#L70-L75
(https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java#L70-L75).
Note that the checkCommitStatusStrict refactor from #12637 does not help
here: checkCommitStatus now delegates to it, but both share the same
commitStatusSupplier, so the NPE happens before the FAILURE vs. UNKNOWN
distinction is ever reached.
This is not Hive-specific in principle — any TableOperations whose doRefresh
leaves currentMetadata null for a not-yet-existing table can hit it — but
HiveTableOperations is the one eproduced here.
## Reproduction
1. Configure a HiveCatalog where HMS cannot service create_table promptly
— e.g. the table LOCATION points at a bucket/path that the metastore process
itself cannot access, so HMS's
Warehouse.isDir/mkdirs blocks and the client hits
hive.metastore.client.socket.timeout. (Any exception reaching the generic catch
(Throwable) in doCommit on a create will do.)
2. CREATE TABLE db.tbl (...) USING iceberg LOCATION '<that path>'
3. Observe the NPE in the status-check retries followed by
CommitStateUnknownException.
Expected: CommitFailedException (or the original exception) reflecting
CommitStatus.FAILURE, since the table demonstrably does not exist in the
catalog.
Actual: three swallowed NPEs and a CommitStateUnknownException
recommending unnecessary manual remediation.
## Test coverage gap
Every Thrift-failure test in TestHiveCommits
(testThriftExceptionUnknownFailedCommit, testThriftExceptionSuccessOnCommit,
testThriftExceptionUnknownStateIfNotInHistoryFailureOnCommit, …)
commits against a table that already exists from setup, i.e. an update,
where currentMetadataLocation is non-null and this path cannot trigger. There
is no test for a commit failure during table creation.
Prior art
PR #6499 (https://github.com/apache/iceberg/pull/6499) ("AWS, Core, Hive:
Fix checkCommitStatus when create table commit fails", @krvikash) diagnosed and
fixed exactly this in Dec 2022,
including tests for Hive and Glue. It received one round of review and was
then closed by the stale bot in Aug 2024 without a decision. There is no open
issue tracking the problem, hence
this report.
One open design question from that PR's review thread is still worth
settling: whether the orphaned metadata JSON written before the failed create
should be cleaned up when the status
resolves to FAILURE.
## Possible fix
Smallest change: null-guard checkCurrentMetadataLocation — if refresh()
yields null metadata, the new location cannot be current or in history, so
return false, which lets checkCommitStatusStrict correctly resolve to FAILURE.
Alternatively, address it a level down so doRefresh doesn't leave the
operations object in a half-refreshed state with shouldRefresh = false and
currentMetadata == null (cf. the older discussion in #1513).
### Willingness to contribute
- [ ] I can contribute a fix for this bug independently
- [ ] I would be willing to contribute a fix for this bug with guidance from
the Iceberg community
- [ ] I cannot contribute a fix for this bug at this time
--
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]
