This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new b9a8632eae [#13224] fix(api): roll back registered model when
registerModel version linking fails (#13225)
b9a8632eae is described below
commit b9a8632eae6dde6e61ce036c37f10fe617241521
Author: YangJie <[email protected]>
AuthorDate: Mon Oct 12 03:04:17 2026 -0400
[#13224] fix(api): roll back registered model when registerModel version
linking fails (#13225)
### What changes were proposed in this pull request?
When version linking fails in the 5-arg default
`ModelCatalog.registerModel`, the just-registered model is now deleted
before the original exception is rethrown, unless it already has a
version, because `deleteModel` also deletes versions. A failure of that
cleanup is attached to the original exception as a suppressed exception.
### Why are the changes needed?
The Java client does not override this method, so it registers the model
and links the version with two REST calls. When the link is rejected,
for example for a numeric alias or a blank URI, the model was left with
zero versions, and retrying the same call failed with
`ModelAlreadyExistsException`.
The check and the delete are not atomic: a version that another caller
links between them is deleted too. The Javadoc says so.
Fix: #13224
### Does this PR introduce _any_ user-facing change?
No API change. A failed 5-arg `registerModel` no longer leaves an empty
model behind.
### How was this patch tested?
`TestModelCatalog` checks that a link failure deletes the empty model
and rethrows the original exception, that a model which already has a
version is kept, that a failed cleanup is attached as suppressed, and
that a model this call did not create is never deleted. The first and
third fail against the code before this change.
---
.../org/apache/gravitino/model/ModelCatalog.java | 20 +-
.../apache/gravitino/model/TestModelCatalog.java | 228 +++++++++++++++++++++
2 files changed, 247 insertions(+), 1 deletion(-)
diff --git a/api/src/main/java/org/apache/gravitino/model/ModelCatalog.java
b/api/src/main/java/org/apache/gravitino/model/ModelCatalog.java
index e10df19ce2..7e6bf02f03 100644
--- a/api/src/main/java/org/apache/gravitino/model/ModelCatalog.java
+++ b/api/src/main/java/org/apache/gravitino/model/ModelCatalog.java
@@ -95,6 +95,12 @@ public interface ModelCatalog {
* model is registered, in the meantime, the model version (version 0) will
also be created and
* linked to the registered model.
*
+ * <p>If linking the model version fails, the model is deleted before the
exception is rethrown,
+ * unless it already has a version, because {@link
#deleteModel(NameIdentifier)} also deletes the
+ * model's versions. The check and the delete are not atomic, so a version
linked between them is
+ * deleted too. If the check or the delete fails, its exception is added to
the rethrown one with
+ * {@link Throwable#addSuppressed(Throwable)}.
+ *
* @param ident The name identifier of the model.
* @param uris The names and URIs of the model version artifact.
* @param aliases The aliases of the model version. The aliases should be
unique in this model,
@@ -118,7 +124,19 @@ public interface ModelCatalog {
throws NoSuchSchemaException, ModelAlreadyExistsException,
ModelVersionAliasesAlreadyExistException {
Model model = registerModel(ident, comment, properties);
- linkModelVersion(ident, uris, aliases, comment, properties);
+ try {
+ linkModelVersion(ident, uris, aliases, comment, properties);
+ } catch (RuntimeException e) {
+ try {
+ // deleteModel also deletes versions, so only roll back a model that
is still empty.
+ if (listModelVersions(ident).length == 0) {
+ deleteModel(ident);
+ }
+ } catch (RuntimeException compensationFailure) {
+ e.addSuppressed(compensationFailure);
+ }
+ throw e;
+ }
return model;
}
diff --git a/api/src/test/java/org/apache/gravitino/model/TestModelCatalog.java
b/api/src/test/java/org/apache/gravitino/model/TestModelCatalog.java
new file mode 100644
index 0000000000..799a10d898
--- /dev/null
+++ b/api/src/test/java/org/apache/gravitino/model/TestModelCatalog.java
@@ -0,0 +1,228 @@
+/*
+ * 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.gravitino.model;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Sets;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.Namespace;
+import org.apache.gravitino.exceptions.ModelAlreadyExistsException;
+import
org.apache.gravitino.exceptions.ModelVersionAliasesAlreadyExistException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestModelCatalog {
+
+ /** A minimal catalog whose linkModelVersion always fails, to exercise the
failure path. */
+ private static class FailingLinkModelCatalog implements ModelCatalog {
+ final Set<NameIdentifier> models = Sets.newHashSet();
+ int deleteModelCalls = 0;
+ int versionsToReport = 0;
+ boolean failDeleteModel = false;
+
+ @Override
+ public Model registerModel(
+ NameIdentifier ident, String comment, Map<String, String> properties) {
+ if (!models.add(ident)) {
+ throw new ModelAlreadyExistsException("model %s already exists",
ident);
+ }
+ return null;
+ }
+
+ @Override
+ public boolean deleteModel(NameIdentifier ident) {
+ deleteModelCalls++;
+ if (failDeleteModel) {
+ throw new RuntimeException("compensation delete failed");
+ }
+ return models.remove(ident);
+ }
+
+ @Override
+ public void linkModelVersion(
+ NameIdentifier ident,
+ Map<String, String> uris,
+ String[] aliases,
+ String comment,
+ Map<String, String> properties)
+ throws ModelVersionAliasesAlreadyExistException {
+ throw new ModelVersionAliasesAlreadyExistException("alias already
exists");
+ }
+
+ @Override
+ public NameIdentifier[] listModels(Namespace namespace) {
+ return new NameIdentifier[0];
+ }
+
+ @Override
+ public Model getModel(NameIdentifier ident) {
+ return null;
+ }
+
+ @Override
+ public int[] listModelVersions(NameIdentifier ident) {
+ return new int[versionsToReport];
+ }
+
+ @Override
+ public ModelVersion[] listModelVersionInfos(NameIdentifier ident) {
+ return new ModelVersion[0];
+ }
+
+ @Override
+ public ModelVersion getModelVersion(NameIdentifier ident, int version) {
+ return null;
+ }
+
+ @Override
+ public ModelVersion getModelVersion(NameIdentifier ident, String alias) {
+ return null;
+ }
+
+ @Override
+ public String getModelVersionUri(NameIdentifier ident, int version, String
uriName) {
+ return null;
+ }
+
+ @Override
+ public String getModelVersionUri(NameIdentifier ident, String alias,
String uriName) {
+ return null;
+ }
+
+ @Override
+ public boolean deleteModelVersion(NameIdentifier ident, int version) {
+ return false;
+ }
+
+ @Override
+ public boolean deleteModelVersion(NameIdentifier ident, String alias) {
+ return false;
+ }
+
+ @Override
+ public Model alterModel(NameIdentifier ident, ModelChange... changes) {
+ return null;
+ }
+
+ @Override
+ public ModelVersion alterModelVersion(
+ NameIdentifier ident, int version, ModelVersionChange... changes) {
+ return null;
+ }
+
+ @Override
+ public ModelVersion alterModelVersion(
+ NameIdentifier ident, String alias, ModelVersionChange... changes) {
+ return null;
+ }
+ }
+
+ @Test
+ void testRegisterModelCleansUpWhenLinkModelVersionFails() {
+ FailingLinkModelCatalog catalog = new FailingLinkModelCatalog();
+ NameIdentifier ident = NameIdentifier.of("schema", "model1");
+
+ Assertions.assertThrows(
+ ModelVersionAliasesAlreadyExistException.class,
+ () ->
+ catalog.registerModel(
+ ident,
+ ImmutableMap.of("uri", "file:///tmp/m"),
+ new String[] {"alias"},
+ "comment",
+ Collections.emptyMap()));
+
+ // Before the fix, the just-registered model was left behind as an orphan
with zero versions
+ // when linking the version failed.
+ Assertions.assertFalse(
+ catalog.models.contains(ident),
+ "failed registerModel must not leave the registered model behind");
+ Assertions.assertEquals(1, catalog.deleteModelCalls);
+ }
+
+ @Test
+ void testRegisterModelDoesNotRollBackModelWithConcurrentVersions() {
+ FailingLinkModelCatalog catalog = new FailingLinkModelCatalog();
+ catalog.versionsToReport = 1;
+ NameIdentifier ident = NameIdentifier.of("schema", "model1");
+
+ Assertions.assertThrows(
+ ModelVersionAliasesAlreadyExistException.class,
+ () ->
+ catalog.registerModel(
+ ident,
+ ImmutableMap.of("uri", "file:///tmp/m"),
+ new String[] {"alias"},
+ "comment",
+ Collections.emptyMap()));
+
+ // Another actor linked a version to the model concurrently; deleteModel
cascades to all
+ // versions, so the rollback must not remove the model in this case.
+ Assertions.assertTrue(catalog.models.contains(ident));
+ Assertions.assertEquals(0, catalog.deleteModelCalls);
+ }
+
+ @Test
+ void testRegisterModelDoesNotDeleteExistingModel() {
+ FailingLinkModelCatalog catalog = new FailingLinkModelCatalog();
+ NameIdentifier ident = NameIdentifier.of("schema", "model1");
+ catalog.models.add(ident);
+
+ // registerModel throws before the rollback can run; a model this call did
not create must stay.
+ Assertions.assertThrows(
+ ModelAlreadyExistsException.class,
+ () ->
+ catalog.registerModel(
+ ident,
+ ImmutableMap.of("uri", "file:///tmp/m"),
+ new String[] {"alias"},
+ "comment",
+ Collections.emptyMap()));
+ Assertions.assertTrue(catalog.models.contains(ident));
+ Assertions.assertEquals(0, catalog.deleteModelCalls);
+ }
+
+ @Test
+ void testRegisterModelKeepsLinkFailureWhenRollbackFails() {
+ FailingLinkModelCatalog catalog = new FailingLinkModelCatalog();
+ catalog.failDeleteModel = true;
+ NameIdentifier ident = NameIdentifier.of("schema", "model1");
+
+ ModelVersionAliasesAlreadyExistException thrown =
+ Assertions.assertThrows(
+ ModelVersionAliasesAlreadyExistException.class,
+ () ->
+ catalog.registerModel(
+ ident,
+ ImmutableMap.of("uri", "file:///tmp/m"),
+ new String[] {"alias"},
+ "comment",
+ Collections.emptyMap()));
+
+ // The original linking failure stays the primary exception; the failed
compensation is
+ // recorded as a suppressed exception rather than discarded.
+ Assertions.assertEquals(1, catalog.deleteModelCalls);
+ Throwable[] suppressed = thrown.getSuppressed();
+ Assertions.assertEquals(1, suppressed.length);
+ Assertions.assertEquals("compensation delete failed",
suppressed[0].getMessage());
+ }
+}
