You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "superdiaodiao (via GitHub)" <gi...@apache.org> on 2026/08/01 09:15:02 UTC
[PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "superdiaodiao (via GitHub)" <gi...@apache.org>.
superdiaodiao commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5584258633
Reworked the tests in 13aa5af — you were right on both counts, and checking them turned up something worth reporting.
**"Does this add value? Is there any change in the actual commit results?"**
No, there isn't. I removed the guard and re-ran on both sides to check:
```
TestHiveCommits.java:649 -> NPE at BaseMetastoreTableOperations.checkCurrentMetadataLocation
TestHiveViewCommits.java:282 -> NPE at HiveViewOperations.checkCurrentMetadataLocation
```
Both failures land on the reflective call, not on the `CommitStateUnknownException` / `tableExists` assertions above it. `checkCommitStatus` swallows the NPE and still reports UNKNOWN, so the observable commit result is identical with and without the fix. The reflection was the only thing seeing a difference — which is exactly your point, so it is gone.
**`@VisibleForTesting` package-private:** works for the view (`HiveViewOperations` and `TestHiveViewCommits` share `org.apache.iceberg.hive`), so that side now calls `ops.checkCurrentMetadataLocation(...)` directly.
It does **not** work for the table side: `checkCurrentMetadataLocation` lives in `org.apache.iceberg.BaseMetastoreTableOperations` while the test is in `org.apache.iceberg.hive`, so package-private is not visible across the boundary. Rather than widen it further, I took your other suggestion: the null-metadata behaviour is covered by `TestBaseMetastoreTableOperations` in the right package, and `TestHiveCommits` now only pins the Hive-specific piece it is actually there for — that `doRefresh` on a never-persisted table yields null metadata rather than throwing. `checkCurrentMetadataLocation` stays private in core.
Net: −33 lines, no reflection.
```
TestBaseMetastoreTableOperations tests=2 failures=0
TestHiveCommits tests=17 failures=0
TestHiveViewCommits tests=13 failures=0
```
> So other catalogs don't check ATM
Correct — no commit-status check at all in `NessieViewOperations`, `JdbcViewOperations` or `RESTViewOperations`, and none in `BaseViewOperations`. Hive is the only view implementation with the retry-on-UNKNOWN path, so it is the only one that can reach this NPE. If another catalog adds that check later it will need the same guard.
Also folded the metadata construction into a single `METADATA` constant as suggested.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3959056225
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCommits.java:
##########
@@ -216,6 +218,78 @@ public void testThriftExceptionUnknownStateIfNotInHistoryFailureOnCommit()
.isEqualTo(2);
}
+ /**
+ * Pretends we throw an unclear error while persisting a create-view commit, for a view that was
+ * never stored in the metastore. The commit status check must resolve cleanly instead of NPE-ing:
+ * the view-specific {@code doRefresh} treats a missing view as non-fatal when no metadata
+ * location is known and refreshes from a null location, so the status-check supplier observes
+ * null current metadata and resolves to false (a new metadata location cannot be current for a
+ * view that does not exist). The relaxed check then maps that to UNKNOWN.
+ */
+ @Test
+ public void testThriftExceptionUnknownStateOnCreateCommitWhenViewNeverPersisted()
+ throws TException, InterruptedException, IOException {
+ TableIdentifier createIdentifier = TableIdentifier.of(NS, "create_commit_failed_view");
+ HiveViewOperations ops = (HiveViewOperations) catalog.newViewOps(createIdentifier);
+ HiveViewOperations spyOps = spy(ops);
+
+ failCommitAndThrowException(spyOps);
+
+ Path createLocation = new Path(viewLocation.getParent(), "create_commit_failed_view");
+ ViewMetadata metadata =
+ ViewMetadata.builder()
+ .setLocation(createLocation.toString())
+ .setProperties(
+ ImmutableMap.of(
+ TableProperties.COMMIT_NUM_STATUS_CHECKS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS, "10",
+ TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS, "100"))
+ .setCurrentVersion(
+ ImmutableViewVersion.builder()
+ .versionId(1)
+ .schemaId(SCHEMA.schemaId())
+ .timestampMillis(System.currentTimeMillis())
+ .defaultNamespace(NS)
+ .putSummary("operation", "create")
+ .addRepresentations(
+ ImmutableSQLViewRepresentation.builder()
+ .sql(VIEW_QUERY)
+ .dialect("hive")
+ .build())
+ .build(),
+ SCHEMA)
+ .build();
+
+ try {
+ assertThatThrownBy(() -> spyOps.commit(null, metadata))
+ .isInstanceOf(CommitStateUnknownException.class)
+ .hasMessageStartingWith("Datacenter on fire");
+
+ assertThat(catalog.viewExists(createIdentifier))
+ .as("The view should not have been created")
+ .isFalse();
+
+ // pins the view-specific doRefresh wiring: a missing view is not an error when no metadata
+ // location is known, so refreshing a never-persisted view must yield null metadata
+ assertThat(ops.refresh())
+ .as("Refreshing a never-persisted view should yield null metadata")
+ .isNull();
+
+ // and the commit status check supplier must resolve to false for the null metadata instead
+ // of throwing an NPE
+ assertThat(
+ ops.checkCurrentMetadataLocation(
+ createLocation + "/metadata/00000-uuid.metadata.json"))
+ .as("A new metadata location cannot be current for a never-persisted view")
+ .isFalse();
Review Comment:
Could we check on the number of calls on refresh? We already has a spyOps, so it is easy to reach, and we don't have to expose internals for it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5597451102
Merged to main.
Thanks @superdiaodiao for the fix and @uros-b for the review!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3956206048
##########
core/src/test/java/org/apache/iceberg/TestBaseMetastoreTableOperations.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.apache.iceberg.BaseMetastoreOperations.CommitStatus;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseMetastoreTableOperations {
+
+ private static final Schema SCHEMA =
+ new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get()));
+
+ private static final Map<String, String> FAST_STATUS_CHECKS =
Review Comment:
Maybe inline these to the metadata constant - but your choice
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3957608579
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -596,6 +600,55 @@ private void concurrentCommitAndThrowException(
.persistTable(any(), anyBoolean(), any());
}
+ /**
+ * Pins the table-specific doRefresh wiring for a never-persisted table: a CREATE TABLE commit
+ * that fails with a non-specific exception must resolve the commit status cleanly instead of
+ * NPE-ing in checkCurrentMetadataLocation (#17462). Pins the Hive-specific doRefresh wiring:
+ * refreshing a never-persisted table yields null metadata. The null-metadata handling itself is
+ * covered by TestBaseMetastoreTableOperations.
+ */
+ @Test
+ public void testThriftExceptionUnknownStateOnCreateCommitWhenTableNeverPersisted()
Review Comment:
This tests now succeeds with and without the change, so we can remove it
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "superdiaodiao (via GitHub)" <gi...@apache.org>.
superdiaodiao commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5588773105
All five in 0a6a783.
- `View creation may not have registered metadata yet.` — right, it was copy-pasted from the table side.
- Dropped the `strictStatus` / `status` wrappers; the tests call `checkCommitStatusStrict` and `checkCommitStatus` directly.
- Both javadocs shortened to your one-liners.
- Removed the `try`/`finally`. You are right that it is redundant — `createLocation` is a sibling of `viewLocation` under the same test root, so it goes with the rest of the temp tree. Dropping it also let `IOException` come off the method signature.
```
TestBaseMetastoreTableOperations tests=2 failures=0
TestHiveCommits tests=16 failures=0
TestHiveViewCommits tests=13 failures=0
./gradlew spotlessCheck passed
```
Net −25 lines. Thanks for the thorough pass — the diff ended up a good deal tighter than what I opened with.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "superdiaodiao (via GitHub)" <gi...@apache.org>.
superdiaodiao commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5190117212
Added the table-side integration test in 639bd29 - thanks for the push, it's a better guard than the stub alone.
`TestHiveCommits.testThriftExceptionUnknownStateOnCreateCommitWhenTableNeverPersisted` drives a failed create through the real `HiveTableOperations` (spied `persistTable` throwing `TException`) and pins the wiring the stub could not: `refresh()` yields null metadata for a never-persisted table, and `checkCurrentMetadataLocation` resolves to false instead of NPE-ing. It fails on `main` without the fix. Together with the view test from the previous round, both `doRefresh` paths are now covered end-to-end, so a future refactor of either would break a test rather than silently reintroduce the NPE.
I kept `TestBaseMetastoreTableOperations` as well - it pins the catalog-agnostic contract (the guard lives in `BaseMetastoreTableOperations`, so any metastore catalog whose `doRefresh` can leave metadata null benefits), while the two Hive tests cover the concrete wiring. Happy to drop it if you'd rather not carry both.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "superdiaodiao (via GitHub)" <gi...@apache.org>.
superdiaodiao commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5588244514
Done in 16d9b53 — and it works better than I expected, so `checkCurrentMetadataLocation` is back to `private` in `HiveViewOperations`. No internals exposed anywhere now.
The count is 2, not 1, and the two call sites are exactly the ones that matter:
```
-> BaseViewOperations.current(BaseViewOperations.java:79)
-> HiveViewOperations.checkCurrentMetadataLocation(HiveViewOperations.java:279)
```
The second is the one under test — reaching it at all proves the status check resolved the null metadata rather than aborting.
It also still distinguishes the fix, which was my main worry about an indirect assertion. Dropping the guard gives **3** calls, because the NPE propagates and the check is retried:
```
Wanted 2 times ... But was 3 times:
-> BaseViewOperations.current(...)
-> HiveViewOperations.checkCurrentMetadataLocation(...)
-> HiveViewOperations.checkCurrentMetadataLocation(...)
```
So `verify(spyOps, times(2)).refresh()` fails on `main` and passes with the fix, same as the direct call did, without touching visibility.
```
TestBaseMetastoreTableOperations tests=2 failures=0
TestHiveCommits tests=16 failures=0
TestHiveViewCommits tests=13 failures=0
./gradlew spotlessCheck passed
```
Thanks for pushing on this one — the test is smaller and the production code is untouched, which is clearly the better shape.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3960156849
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCommits.java:
##########
@@ -216,6 +220,68 @@ public void testThriftExceptionUnknownStateIfNotInHistoryFailureOnCommit()
.isEqualTo(2);
}
+ /**
+ * Pretends we throw an unclear error while persisting a create-view commit, for a view that was
+ * never stored in the metastore. The commit status check must resolve cleanly instead of NPE-ing:
+ * the view-specific {@code doRefresh} treats a missing view as non-fatal when no metadata
+ * location is known and refreshes from a null location, so the status-check supplier observes
+ * null current metadata and resolves to false (a new metadata location cannot be current for a
+ * view that does not exist). The relaxed check then maps that to UNKNOWN.
+ */
+ @Test
+ public void testThriftExceptionUnknownStateOnCreateCommitWhenViewNeverPersisted()
+ throws TException, InterruptedException, IOException {
+ TableIdentifier createIdentifier = TableIdentifier.of(NS, "create_commit_failed_view");
+ HiveViewOperations ops = (HiveViewOperations) catalog.newViewOps(createIdentifier);
+ HiveViewOperations spyOps = spy(ops);
+
+ failCommitAndThrowException(spyOps);
+
+ Path createLocation = new Path(viewLocation.getParent(), "create_commit_failed_view");
+ ViewMetadata metadata =
+ ViewMetadata.builder()
+ .setLocation(createLocation.toString())
+ .setProperties(
+ ImmutableMap.of(
+ TableProperties.COMMIT_NUM_STATUS_CHECKS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS, "10",
+ TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS, "100"))
+ .setCurrentVersion(
+ ImmutableViewVersion.builder()
+ .versionId(1)
+ .schemaId(SCHEMA.schemaId())
+ .timestampMillis(System.currentTimeMillis())
+ .defaultNamespace(NS)
+ .putSummary("operation", "create")
+ .addRepresentations(
+ ImmutableSQLViewRepresentation.builder()
+ .sql(VIEW_QUERY)
+ .dialect("hive")
+ .build())
+ .build(),
+ SCHEMA)
+ .build();
+
+ try {
+ assertThatThrownBy(() -> spyOps.commit(null, metadata))
+ .isInstanceOf(CommitStateUnknownException.class)
+ .hasMessageStartingWith("Datacenter on fire");
+
+ assertThat(catalog.viewExists(createIdentifier))
+ .as("The view should not have been created")
+ .isFalse();
+
+ // the configured status check must run to completion: once from current(), once from
+ // inside checkCurrentMetadataLocation, which resolves the null metadata instead of throwing
+ verify(spyOps, times(2)).refresh();
+ } finally {
+ createLocation
Review Comment:
I think this is not needed. Isn't `@After` takes care of this?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "superdiaodiao (via GitHub)" <gi...@apache.org>.
superdiaodiao commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5339471242
@uros-b gentle follow-up — the table-side integration test you asked for went in as 639bd29 two weeks ago, so both of your comments are addressed and CI is green.
Anything else you would like changed here, or is this ready for another look?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3956185931
##########
core/src/test/java/org/apache/iceberg/TestBaseMetastoreTableOperations.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.apache.iceberg.BaseMetastoreOperations.CommitStatus;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseMetastoreTableOperations {
Review Comment:
why public? Package private is enough
##########
core/src/test/java/org/apache/iceberg/TestBaseMetastoreTableOperations.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.apache.iceberg.BaseMetastoreOperations.CommitStatus;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseMetastoreTableOperations {
+
+ private static final Schema SCHEMA =
+ new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get()));
+
+ private static final Map<String, String> FAST_STATUS_CHECKS =
+ ImmutableMap.of(
+ TableProperties.COMMIT_NUM_STATUS_CHECKS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS, "10",
+ TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS, "100");
+
+ /**
+ * Mimics metastore-backed table operations for a table that was never persisted to the catalog,
+ * e.g. when a CREATE TABLE commit fails before the table is stored in the metastore. Like {@code
+ * HiveTableOperations#doRefresh()}, a missing table is not an error when no metadata location is
+ * known, and refreshing from a null metadata location leaves the current metadata null.
+ */
+ private static class NeverPersistedTableOperations extends BaseMetastoreTableOperations {
+
+ @Override
+ protected String tableName() {
+ return "db.never_persisted";
+ }
+
+ @Override
+ public FileIO io() {
+ return null;
+ }
+
+ @Override
+ protected void doRefresh() {
+ refreshFromMetadataLocation(null, 1);
+ }
+
+ private CommitStatus strictStatus(String newMetadataLocation, TableMetadata config) {
+ return checkCommitStatusStrict(newMetadataLocation, config);
+ }
+
+ private CommitStatus status(String newMetadataLocation, TableMetadata config) {
+ return checkCommitStatus(newMetadataLocation, config);
+ }
+ }
+
+ @Test
+ public void strictStatusCheckIsFailureWhenTableWasNeverPersisted() {
Review Comment:
package private
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3956307837
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveViewOperations.java:
##########
@@ -276,6 +276,11 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) {
*/
private boolean checkCurrentMetadataLocation(String newMetadataLocation) {
ViewMetadata metadata = refresh();
+ if (metadata == null) {
Review Comment:
Is it enough to do it in the Hive codepath? What about other catalogs?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3956347020
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -596,6 +600,63 @@ private void concurrentCommitAndThrowException(
.persistTable(any(), anyBoolean(), any());
}
+ /**
+ * Pins the table-specific doRefresh wiring for a never-persisted table: a CREATE TABLE commit
+ * that fails with a non-specific exception must resolve the commit status cleanly instead of
+ * NPE-ing in checkCurrentMetadataLocation (#17462). refresh() yields null current metadata and
+ * the status check supplier resolves to false (a new metadata location cannot be current for a
+ * table that does not exist). The relaxed check then maps that to UNKNOWN.
+ */
+ @Test
+ public void testThriftExceptionUnknownStateOnCreateCommitWhenTableNeverPersisted()
+ throws TException, InterruptedException, IOException {
+ TableIdentifier createIdentifier = TableIdentifier.of(DB_NAME, "create_commit_failed_table");
+ HiveTableOperations ops = (HiveTableOperations) catalog.newTableOps(createIdentifier);
+ HiveTableOperations spyOps = spy(ops);
+
+ failCommitAndThrowException(spyOps);
+
+ Path createLocation = getTableLocationPath("create_commit_failed_table");
+ TableMetadata metadata =
+ TableMetadata.newTableMetadata(
+ SCHEMA,
+ PartitionSpec.unpartitioned(),
+ createLocation.toString(),
+ ImmutableMap.of(
+ TableProperties.COMMIT_NUM_STATUS_CHECKS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS, "10",
+ TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS, "100"));
+
+ try {
+ assertThatThrownBy(() -> spyOps.commit(null, metadata))
+ .isInstanceOf(CommitStateUnknownException.class)
+ .hasMessageStartingWith("Datacenter on fire");
+
+ assertThat(catalog.tableExists(createIdentifier))
+ .as("The table should not have been created")
+ .isFalse();
+
+ // pins the table-specific doRefresh wiring: a missing table is not an error when no
+ // metadata location is known, so refreshing a never-persisted table must yield null metadata
+ assertThat(ops.refresh())
+ .as("Refreshing a never-persisted table should yield null metadata")
+ .isNull();
+
+ // and the commit status check supplier must resolve to false for the null metadata instead
+ // of throwing an NPE
+ assertThat(
+ checkCurrentMetadataLocation(
+ ops, createLocation + "/metadata/00000-uuid.metadata.json"))
+ .as("A new metadata location cannot be current for a never-persisted table")
Review Comment:
Does this add value?
Is there any change in the actual commit results?
Shall we just create an TestHiveTableOperations if we want to ensure that the HiveTableOperations does the same as the BaseMetastoreTableOperations?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3960124511
##########
core/src/test/java/org/apache/iceberg/TestBaseMetastoreTableOperations.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.apache.iceberg.BaseMetastoreOperations.CommitStatus;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+class TestBaseMetastoreTableOperations {
+
+ private static final String TABLE_LOCATION = "file:/tmp/db/never_persisted";
+ private static final String METADATA_LOCATION =
+ TABLE_LOCATION + "/metadata/00000-uuid.metadata.json";
+
+ private static final TableMetadata METADATA =
+ TableMetadata.newTableMetadata(
+ new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get())),
+ PartitionSpec.unpartitioned(),
+ TABLE_LOCATION,
+ ImmutableMap.of(
+ TableProperties.COMMIT_NUM_STATUS_CHECKS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS, "10",
+ TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS, "100"));
+
+ /**
+ * Mimics metastore-backed table operations for a table that was never persisted to the catalog,
+ * e.g. when a CREATE TABLE commit fails before the table is stored in the metastore. Like {@code
+ * HiveTableOperations#doRefresh()}, a missing table is not an error when no metadata location is
+ * known, and refreshing from a null metadata location leaves the current metadata null.
+ */
Review Comment:
nit: Shorten, like
```
/** Table operations for a table that was never persisted, so refresh yields null metadata. */
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5580519880
Thank you @superdiaodiao for working on this and addressing previous comments! Please ping the expert Iceberg committers for further PR review
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3960109563
##########
core/src/test/java/org/apache/iceberg/TestBaseMetastoreTableOperations.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.apache.iceberg.BaseMetastoreOperations.CommitStatus;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+class TestBaseMetastoreTableOperations {
+
+ private static final String TABLE_LOCATION = "file:/tmp/db/never_persisted";
+ private static final String METADATA_LOCATION =
+ TABLE_LOCATION + "/metadata/00000-uuid.metadata.json";
+
+ private static final TableMetadata METADATA =
+ TableMetadata.newTableMetadata(
+ new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get())),
+ PartitionSpec.unpartitioned(),
+ TABLE_LOCATION,
+ ImmutableMap.of(
+ TableProperties.COMMIT_NUM_STATUS_CHECKS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS, "10",
+ TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS, "100"));
+
+ /**
+ * Mimics metastore-backed table operations for a table that was never persisted to the catalog,
+ * e.g. when a CREATE TABLE commit fails before the table is stored in the metastore. Like {@code
+ * HiveTableOperations#doRefresh()}, a missing table is not an error when no metadata location is
+ * known, and refreshing from a null metadata location leaves the current metadata null.
+ */
+ private static class NeverPersistedTableOperations extends BaseMetastoreTableOperations {
+
+ @Override
+ protected String tableName() {
+ return "db.never_persisted";
+ }
+
+ @Override
+ public FileIO io() {
+ return null;
+ }
+
+ @Override
+ protected void doRefresh() {
+ refreshFromMetadataLocation(null, 1);
+ }
+
+ private CommitStatus strictStatus(String newMetadataLocation, TableMetadata config) {
+ return checkCommitStatusStrict(newMetadataLocation, config);
+ }
+
+ private CommitStatus status(String newMetadataLocation, TableMetadata config) {
+ return checkCommitStatus(newMetadataLocation, config);
+ }
Review Comment:
nit: We don't need these methods. They are protected, we can call them directly in the tests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "superdiaodiao (via GitHub)" <gi...@apache.org>.
superdiaodiao commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5580680592
Thanks @uros-b for the reviews and for confirming the follow-ups.
@nastra @pvary @nk1506 — would one of you be able to take a look? Picking you three because this spans both modules the fix touches: `nastra` and `nk1506` have the most recent history on `BaseMetastoreTableOperations`, `pvary` and `nk1506` on the Hive side.
**What it fixes.** When a create commit fails before the table or view has ever been persisted, `checkCurrentMetadataLocation` is reached with null metadata and NPEs. The commit status check then reports `FAILURE` instead of `UNKNOWN`, so a create that may actually have succeeded server-side is not retried.
**Change.** A null guard in `BaseMetastoreTableOperations.checkCurrentMetadataLocation`, plus the equivalent in `HiveViewOperations`.
**Tests**, both added in response to @uros-b's review and both failing on `main` without the fix:
- `TestHiveCommits.testThriftExceptionUnknownStateOnCreateCommitWhenTableNeverPersisted` (639bd29) — drives a failed create through the real `HiveTableOperations` with a spied `persistTable` throwing `TException`
- `TestHiveViewCommits` equivalent for the create-view path (7105ad0)
CI is green. Happy to rebase or adjust scope if you'd prefer it split differently.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3956090303
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##########
@@ -333,6 +333,13 @@ protected CommitStatus checkCommitStatusStrict(String newMetadataLocation, Table
*/
private boolean checkCurrentMetadataLocation(String newMetadataLocation) {
TableMetadata metadata = refresh();
+ if (metadata == null) {
+ // the table does not exist in the catalog. this happens when a create-table commit fails
+ // before the table is persisted, in which case the new metadata location cannot be the
+ // current one or part of the table's history
Review Comment:
This is too long
```suggestion
// Table creation may not have registered metadata yet.
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3956093009
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveViewOperations.java:
##########
@@ -276,6 +276,13 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) {
*/
private boolean checkCurrentMetadataLocation(String newMetadataLocation) {
ViewMetadata metadata = refresh();
+ if (metadata == null) {
+ // the view does not exist in the catalog. this happens when a create-view commit fails
+ // before the view is persisted, in which case the new metadata location cannot be the
+ // current one
Review Comment:
```suggestion
// Table creation may not have registered metadata yet.
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "superdiaodiao (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "superdiaodiao (via GitHub)" <gi...@apache.org>.
superdiaodiao commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5578657782
Still relevant — flagging so this does not go stale.
Both of @uros-b's review comments are addressed: 7105ad0 added the `TestHiveViewCommits` coverage for the create-view path, and 639bd29 added `TestHiveCommits.testThriftExceptionUnknownStateOnCreateCommitWhenTableNeverPersisted`, which drives a failed create through the real `HiveTableOperations`. Both fail on `main` without the fix, and CI is green.
For context on the duplicate: #17548 proposes the same one-line guard without tests and is now going stale too, so the underlying bug — a failed create commit reporting FAILURE instead of UNKNOWN because `checkCurrentMetadataLocation` NPEs on null metadata — is currently unfixed on `main` either way.
I have also raised this on dev@iceberg.apache.org. Happy to rebase or adjust anything if a committer has a moment to look.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3956188989
##########
core/src/test/java/org/apache/iceberg/TestBaseMetastoreTableOperations.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.apache.iceberg.BaseMetastoreOperations.CommitStatus;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseMetastoreTableOperations {
+
+ private static final Schema SCHEMA =
+ new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get()));
+
+ private static final Map<String, String> FAST_STATUS_CHECKS =
+ ImmutableMap.of(
+ TableProperties.COMMIT_NUM_STATUS_CHECKS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS, "10",
+ TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS, "100");
+
+ /**
+ * Mimics metastore-backed table operations for a table that was never persisted to the catalog,
+ * e.g. when a CREATE TABLE commit fails before the table is stored in the metastore. Like {@code
+ * HiveTableOperations#doRefresh()}, a missing table is not an error when no metadata location is
+ * known, and refreshing from a null metadata location leaves the current metadata null.
+ */
+ private static class NeverPersistedTableOperations extends BaseMetastoreTableOperations {
+
+ @Override
+ protected String tableName() {
+ return "db.never_persisted";
+ }
+
+ @Override
+ public FileIO io() {
+ return null;
+ }
+
+ @Override
+ protected void doRefresh() {
+ refreshFromMetadataLocation(null, 1);
+ }
+
+ private CommitStatus strictStatus(String newMetadataLocation, TableMetadata config) {
+ return checkCommitStatusStrict(newMetadataLocation, config);
+ }
+
+ private CommitStatus status(String newMetadataLocation, TableMetadata config) {
+ return checkCommitStatus(newMetadataLocation, config);
+ }
+ }
+
+ @Test
+ public void strictStatusCheckIsFailureWhenTableWasNeverPersisted() {
+ NeverPersistedTableOperations ops = new NeverPersistedTableOperations();
+ TableMetadata metadata =
+ TableMetadata.newTableMetadata(
+ SCHEMA,
+ PartitionSpec.unpartitioned(),
+ "file:/tmp/db/never_persisted",
+ FAST_STATUS_CHECKS);
+
+ assertThat(
+ ops.strictStatus(
+ "file:/tmp/db/never_persisted/metadata/00000-uuid.metadata.json", metadata))
+ .isEqualTo(CommitStatus.FAILURE);
+ }
+
+ @Test
+ public void statusCheckIsUnknownWhenTableWasNeverPersisted() {
Review Comment:
package private
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3957624732
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCommits.java:
##########
@@ -594,6 +668,7 @@ private void breakFallbackCatalogCommitCheck(HiveViewOperations spyOperations) {
.thenThrow(new RuntimeException("Still on fire")); // Failure on commit check
}
+
Review Comment:
remove extra new line
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "superdiaodiao (via GitHub)" <gi...@apache.org>.
superdiaodiao commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5584856406
Both done in ad04f30 — table-side test removed, extra blank line gone.
Before removing it I checked the view side still earns its place, since the two tests looked symmetric. It does: dropping the guard from `HiveViewOperations` still fails `TestHiveViewCommits`, because that assertion now calls `checkCurrentMetadataLocation` directly rather than through reflection, and the method is package-private to the same package. So the view test distinguishes the fix and the table one did not — exactly the asymmetry you spotted.
Coverage after the change:
- `TestBaseMetastoreTableOperations` — null-metadata handling on the table path, in the package that owns it
- `TestHiveViewCommits` — the view path end-to-end, fails without the guard
- `TestHiveCommits` — untouched Hive commit behaviour, minus the test that proved nothing
```
TestBaseMetastoreTableOperations tests=2 failures=0
TestHiveCommits tests=16 failures=0
TestHiveViewCommits tests=13 failures=0
./gradlew spotlessCheck passed
```
`spotlessApply` also dropped a now-unused `java.util.Map` import from the core test, which is in the same commit.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "superdiaodiao (via GitHub)" <gi...@apache.org>.
superdiaodiao commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5163209450
Added a `TestHiveViewCommits` case for the create-view-commit-fails path in 7105ad0. It drives a failed create through the real `HiveViewOperations` (spied `persistTable` throwing `TException`), asserts the relaxed status check maps to UNKNOWN, and pins the view-specific `doRefresh` wiring: `refresh()` yields null metadata for a never-persisted view and `checkCurrentMetadataLocation` resolves to false instead of NPE-ing. The reflection-based supplier assertion fails on main without the fix.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3956388412
##########
core/src/test/java/org/apache/iceberg/TestBaseMetastoreTableOperations.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.apache.iceberg.BaseMetastoreOperations.CommitStatus;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+class TestBaseMetastoreTableOperations {
+
+ private static final Schema SCHEMA =
+ new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get()));
+
+ private static final String TABLE_LOCATION = "file:/tmp/db/never_persisted";
+ private static final String METADATA_LOCATION =
+ TABLE_LOCATION + "/metadata/00000-uuid.metadata.json";
+
+ private static final Map<String, String> FAST_STATUS_CHECKS =
+ ImmutableMap.of(
+ TableProperties.COMMIT_NUM_STATUS_CHECKS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS, "10",
+ TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS, "100");
+
+ /**
+ * Mimics metastore-backed table operations for a table that was never persisted to the catalog,
+ * e.g. when a CREATE TABLE commit fails before the table is stored in the metastore. Like {@code
+ * HiveTableOperations#doRefresh()}, a missing table is not an error when no metadata location is
+ * known, and refreshing from a null metadata location leaves the current metadata null.
+ */
+ private static class NeverPersistedTableOperations extends BaseMetastoreTableOperations {
+
+ @Override
+ protected String tableName() {
+ return "db.never_persisted";
+ }
+
+ @Override
+ public FileIO io() {
+ return null;
+ }
+
+ @Override
+ protected void doRefresh() {
+ refreshFromMetadataLocation(null, 1);
+ }
+
+ private CommitStatus strictStatus(String newMetadataLocation, TableMetadata config) {
+ return checkCommitStatusStrict(newMetadataLocation, config);
+ }
+
+ private CommitStatus status(String newMetadataLocation, TableMetadata config) {
+ return checkCommitStatus(newMetadataLocation, config);
+ }
+ }
+
+ @Test
+ void strictStatusCheckIsFailureWhenTableWasNeverPersisted() {
+ NeverPersistedTableOperations ops = new NeverPersistedTableOperations();
+ TableMetadata metadata =
+ TableMetadata.newTableMetadata(
+ SCHEMA,
+ PartitionSpec.unpartitioned(),
+ TABLE_LOCATION,
+ FAST_STATUS_CHECKS);
+
+ assertThat(
+ ops.strictStatus(
+ METADATA_LOCATION, metadata))
+ .isEqualTo(CommitStatus.FAILURE);
+ }
+
+ @Test
+ void statusCheckIsUnknownWhenTableWasNeverPersisted() {
+ NeverPersistedTableOperations ops = new NeverPersistedTableOperations();
+ TableMetadata metadata =
+ TableMetadata.newTableMetadata(
+ SCHEMA,
+ PartitionSpec.unpartitioned(),
+ TABLE_LOCATION,
+ FAST_STATUS_CHECKS);
Review Comment:
```
private static final TableMetadata METADATA =
TableMetadata.newTableMetadata(
new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get())),
PartitionSpec.unpartitioned(),
TABLE_LOCATION,
ImmutableMap.of(
TableProperties.COMMIT_NUM_STATUS_CHECKS, "1",
TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS, "1",
TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS, "10",
TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS, "100"));
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3956196333
##########
core/src/test/java/org/apache/iceberg/TestBaseMetastoreTableOperations.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.apache.iceberg.BaseMetastoreOperations.CommitStatus;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+public class TestBaseMetastoreTableOperations {
+
+ private static final Schema SCHEMA =
+ new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get()));
+
+ private static final Map<String, String> FAST_STATUS_CHECKS =
+ ImmutableMap.of(
+ TableProperties.COMMIT_NUM_STATUS_CHECKS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS, "1",
+ TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS, "10",
+ TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS, "100");
+
+ /**
+ * Mimics metastore-backed table operations for a table that was never persisted to the catalog,
+ * e.g. when a CREATE TABLE commit fails before the table is stored in the metastore. Like {@code
+ * HiveTableOperations#doRefresh()}, a missing table is not an error when no metadata location is
+ * known, and refreshing from a null metadata location leaves the current metadata null.
+ */
+ private static class NeverPersistedTableOperations extends BaseMetastoreTableOperations {
+
+ @Override
+ protected String tableName() {
+ return "db.never_persisted";
+ }
+
+ @Override
+ public FileIO io() {
+ return null;
+ }
+
+ @Override
+ protected void doRefresh() {
+ refreshFromMetadataLocation(null, 1);
+ }
+
+ private CommitStatus strictStatus(String newMetadataLocation, TableMetadata config) {
+ return checkCommitStatusStrict(newMetadataLocation, config);
+ }
+
+ private CommitStatus status(String newMetadataLocation, TableMetadata config) {
+ return checkCommitStatus(newMetadataLocation, config);
+ }
+ }
+
+ @Test
+ public void strictStatusCheckIsFailureWhenTableWasNeverPersisted() {
+ NeverPersistedTableOperations ops = new NeverPersistedTableOperations();
+ TableMetadata metadata =
Review Comment:
Can we make this a constant?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "superdiaodiao (via GitHub)" <gi...@apache.org>.
superdiaodiao commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5582264725
Thanks for the quick look @pvary — both suggestions applied verbatim in c7bc14e.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3960128330
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCommits.java:
##########
@@ -216,6 +220,68 @@ public void testThriftExceptionUnknownStateIfNotInHistoryFailureOnCommit()
.isEqualTo(2);
}
+ /**
+ * Pretends we throw an unclear error while persisting a create-view commit, for a view that was
+ * never stored in the metastore. The commit status check must resolve cleanly instead of NPE-ing:
+ * the view-specific {@code doRefresh} treats a missing view as non-fatal when no metadata
+ * location is known and refreshes from a null location, so the status-check supplier observes
+ * null current metadata and resolves to false (a new metadata location cannot be current for a
+ * view that does not exist). The relaxed check then maps that to UNKNOWN.
+ */
Review Comment:
nit: trim, like:
```
/** Pretends we throw an unclear error while persisting a create-view commit for a new view. */
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5159350133
Also, no corresponding test in TestHiveViewCommits for the create-view-commit-fails path. The HiveViewOperations null guard is only transitively covered by the base-class stub; a parallel view test would pin the view-specific doRefresh wiring against future refactors.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3956395573
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveViewOperations.java:
##########
@@ -276,6 +276,11 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) {
*/
private boolean checkCurrentMetadataLocation(String newMetadataLocation) {
ViewMetadata metadata = refresh();
+ if (metadata == null) {
Review Comment:
So other catalogs don't check ATM
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#issuecomment-5588578675
Thanks @superdiaodiao for the PR, I think we are getting there soon
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3960100654
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveViewOperations.java:
##########
@@ -276,6 +276,11 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) {
*/
private boolean checkCurrentMetadataLocation(String newMetadataLocation) {
ViewMetadata metadata = refresh();
+ if (metadata == null) {
+ // Table creation may not have registered metadata yet.
Review Comment:
nit: `View` creation
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17464:
URL: https://github.com/apache/iceberg/pull/17464#discussion_r3956321197
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -608,6 +669,20 @@ private void breakFallbackCatalogCommitCheck(HiveTableOperations spyOperations)
.thenThrow(new RuntimeException("Still on fire")); // Failure on commit check
}
+ private static boolean checkCurrentMetadataLocation(
Review Comment:
Could this be solved making `checkCurrentMetadataLocation` package private with `@VisibleForTesting` annotation?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Fix NPE in commit status check when a create commit fails [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #17464:
URL: https://github.com/apache/iceberg/pull/17464
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org