From c0b57dc3d00bc430832b057f87871a97fa19fe46 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Tue, 1 Jul 2025 14:22:22 +0100 Subject: [PATCH 1/7] Concurrent Namespace Update Fails with RuntimeException and 500 Error #1390 --- .../polaris/service/catalog/iceberg/IcebergCatalog.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index f8b7edf337..cdb001535b 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -724,7 +724,7 @@ public boolean setProperties(Namespace namespace, Map properties .map(PolarisEntity::new) .orElse(null); if (returnedEntity == null) { - throw new RuntimeException("Concurrent modification of namespace: " + namespace); + throw new CommitFailedException("Concurrent modification of namespace: %s", namespace); } return true; } @@ -756,7 +756,7 @@ public boolean removeProperties(Namespace namespace, Set properties) .map(PolarisEntity::new) .orElse(null); if (returnedEntity == null) { - throw new RuntimeException("Concurrent modification of namespace: " + namespace); + throw new CommitFailedException("Concurrent modification of namespace: %s", namespace); } return true; } From 78b18e498e73290839203d9a616544f2553f5233 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Wed, 2 Jul 2025 16:14:13 +0100 Subject: [PATCH 2/7] Concurrent Namespace Update Fails with RuntimeException and 500 Error #1390: updated unit tests --- .../quarkus/catalog/IcebergCatalogTest.java | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java index 05c2642f16..7d10742d15 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java @@ -19,8 +19,10 @@ package org.apache.polaris.service.quarkus.catalog; import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.polaris.core.persistence.dao.entity.BaseResult.ReturnStatus.ENTITY_NOT_FOUND; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Fail.fail; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.isA; import static org.mockito.Mockito.doReturn; @@ -486,6 +488,30 @@ public Map purgeRealms(Iterable realms) { }; } + + @Test + public void testPropertiesUpdateConcurrentException() { + PolarisMetaStoreManager spyMetaStore = spy(metaStoreManager); + PolarisPassthroughResolutionView passthroughView = + new PolarisPassthroughResolutionView( + polarisContext, entityManager, securityContext, CATALOG_NAME); + IcebergCatalog catalog = new IcebergCatalog( + entityManager, + spyMetaStore, + polarisContext, + passthroughView, + securityContext, + Mockito.mock(TaskExecutor.class), + fileIOFactory, + polarisEventListener); + catalog.createNamespace(NS); + doReturn(new EntityResult(ENTITY_NOT_FOUND,"")) + .when(spyMetaStore) + .updateEntityPropertiesIfNotChanged(any(), any(), any()); + assertThrows(CommitFailedException.class, () -> catalog.setProperties(NS, ImmutableMap.of("a", "b"))); + assertThrows(CommitFailedException.class, () -> catalog.removeProperties(NS, Set.of("a", "b"))); + } + @Test public void testEmptyNamespace() { IcebergCatalog catalog = catalog(); From fd6b84827fd46d7266f1a2cc0d59a1075490ed61 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Wed, 2 Jul 2025 16:14:55 +0100 Subject: [PATCH 3/7] Concurrent Namespace Update Fails with RuntimeException and 500 Error #1390: updated unit tests --- .../quarkus/catalog/IcebergCatalogTest.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java index 7d10742d15..00c725563f 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java @@ -488,14 +488,14 @@ public Map purgeRealms(Iterable realms) { }; } - @Test public void testPropertiesUpdateConcurrentException() { PolarisMetaStoreManager spyMetaStore = spy(metaStoreManager); PolarisPassthroughResolutionView passthroughView = - new PolarisPassthroughResolutionView( - polarisContext, entityManager, securityContext, CATALOG_NAME); - IcebergCatalog catalog = new IcebergCatalog( + new PolarisPassthroughResolutionView( + polarisContext, entityManager, securityContext, CATALOG_NAME); + IcebergCatalog catalog = + new IcebergCatalog( entityManager, spyMetaStore, polarisContext, @@ -505,10 +505,11 @@ public void testPropertiesUpdateConcurrentException() { fileIOFactory, polarisEventListener); catalog.createNamespace(NS); - doReturn(new EntityResult(ENTITY_NOT_FOUND,"")) - .when(spyMetaStore) - .updateEntityPropertiesIfNotChanged(any(), any(), any()); - assertThrows(CommitFailedException.class, () -> catalog.setProperties(NS, ImmutableMap.of("a", "b"))); + doReturn(new EntityResult(ENTITY_NOT_FOUND, "")) + .when(spyMetaStore) + .updateEntityPropertiesIfNotChanged(any(), any(), any()); + assertThrows( + CommitFailedException.class, () -> catalog.setProperties(NS, ImmutableMap.of("a", "b"))); assertThrows(CommitFailedException.class, () -> catalog.removeProperties(NS, Set.of("a", "b"))); } From d3e28598a1395a4cfa7f99103b1f97dc6b6a5f0a Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Tue, 1 Jul 2025 14:22:22 +0100 Subject: [PATCH 4/7] Concurrent Namespace Update Fails with RuntimeException and 500 Error #1390 --- .../polaris/service/catalog/iceberg/IcebergCatalog.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index 031c3882f1..c8b931e2a1 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -713,7 +713,7 @@ public boolean setProperties(Namespace namespace, Map properties .map(PolarisEntity::new) .orElse(null); if (returnedEntity == null) { - throw new RuntimeException("Concurrent modification of namespace: " + namespace); + throw new CommitFailedException("Concurrent modification of namespace: %s", namespace); } return true; } @@ -745,7 +745,7 @@ public boolean removeProperties(Namespace namespace, Set properties) .map(PolarisEntity::new) .orElse(null); if (returnedEntity == null) { - throw new RuntimeException("Concurrent modification of namespace: " + namespace); + throw new CommitFailedException("Concurrent modification of namespace: %s", namespace); } return true; } From d8ce2a7d75aebb40afbeac4ad4bf719d50466455 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Fri, 18 Jul 2025 14:36:04 +0100 Subject: [PATCH 5/7] Concurrent Namespace Update Fails with RuntimeException and 500 Error #1390 --- .../exceptions/CommitConflictException.java | 38 +++++++++++++++++++ .../catalog/iceberg/IcebergCatalog.java | 5 ++- .../exception/PolarisExceptionMapper.java | 31 +++++++-------- .../exception/ExceptionMapperTest.java | 10 +++++ 4 files changed, 65 insertions(+), 19 deletions(-) create mode 100644 polaris-core/src/main/java/org/apache/polaris/core/exceptions/CommitConflictException.java diff --git a/polaris-core/src/main/java/org/apache/polaris/core/exceptions/CommitConflictException.java b/polaris-core/src/main/java/org/apache/polaris/core/exceptions/CommitConflictException.java new file mode 100644 index 0000000000..75a61204e2 --- /dev/null +++ b/polaris-core/src/main/java/org/apache/polaris/core/exceptions/CommitConflictException.java @@ -0,0 +1,38 @@ +/* + * 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.polaris.core.exceptions; + +import com.google.errorprone.annotations.FormatMethod; + +public class CommitConflictException extends PolarisException { + @FormatMethod + public CommitConflictException(String message, Object... args) { + super(String.format(message, args)); + } + + @FormatMethod + public CommitConflictException(Throwable cause, String message, Object... args) { + super(String.format(message, args), cause); + } + + public CommitConflictException(String message, Throwable cause) { + super(message, cause); + } +} diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index c8b931e2a1..98828f984d 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -104,6 +104,7 @@ import org.apache.polaris.core.entity.PolarisEntityType; import org.apache.polaris.core.entity.PolarisTaskConstants; import org.apache.polaris.core.entity.table.IcebergTableLikeEntity; +import org.apache.polaris.core.exceptions.CommitConflictException; import org.apache.polaris.core.persistence.PolarisEntityManager; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; @@ -713,7 +714,7 @@ public boolean setProperties(Namespace namespace, Map properties .map(PolarisEntity::new) .orElse(null); if (returnedEntity == null) { - throw new CommitFailedException("Concurrent modification of namespace: %s", namespace); + throw new CommitConflictException("Concurrent modification of namespace: %s", namespace); } return true; } @@ -745,7 +746,7 @@ public boolean removeProperties(Namespace namespace, Set properties) .map(PolarisEntity::new) .orElse(null); if (returnedEntity == null) { - throw new CommitFailedException("Concurrent modification of namespace: %s", namespace); + throw new CommitConflictException("Concurrent modification of namespace: %s", namespace); } return true; } diff --git a/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java b/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java index dfbd4f6c5c..4832ee4e71 100644 --- a/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java @@ -24,6 +24,7 @@ import jakarta.ws.rs.ext.Provider; import org.apache.iceberg.rest.responses.ErrorResponse; import org.apache.polaris.core.exceptions.AlreadyExistsException; +import org.apache.polaris.core.exceptions.CommitConflictException; import org.apache.polaris.core.exceptions.PolarisException; import org.apache.polaris.core.persistence.PolicyMappingAlreadyExistsException; import org.apache.polaris.core.policy.exceptions.NoSuchPolicyException; @@ -45,23 +46,19 @@ public class PolarisExceptionMapper implements ExceptionMapper private static final Logger LOGGER = LoggerFactory.getLogger(PolarisExceptionMapper.class); private Response.Status getStatus(PolarisException exception) { - if (exception instanceof AlreadyExistsException) { - return Response.Status.CONFLICT; - } else if (exception instanceof InvalidPolicyException) { - return Response.Status.BAD_REQUEST; - } else if (exception instanceof PolicyAttachException) { - return Response.Status.BAD_REQUEST; - } else if (exception instanceof NoSuchPolicyException) { - return Response.Status.NOT_FOUND; - } else if (exception instanceof PolicyVersionMismatchException) { - return Response.Status.CONFLICT; - } else if (exception instanceof PolicyMappingAlreadyExistsException) { - return Response.Status.CONFLICT; - } else if (exception instanceof PolicyInUseException) { - return Response.Status.BAD_REQUEST; - } else { - return Response.Status.INTERNAL_SERVER_ERROR; - } + return switch (exception) { + case AlreadyExistsException alreadyExistsException -> Response.Status.CONFLICT; + case CommitConflictException commitConflictException -> Response.Status.REQUEST_TIMEOUT; + case InvalidPolicyException invalidPolicyException -> Response.Status.BAD_REQUEST; + case PolicyAttachException policyAttachException -> Response.Status.BAD_REQUEST; + case NoSuchPolicyException noSuchPolicyException -> Response.Status.NOT_FOUND; + case PolicyVersionMismatchException policyVersionMismatchException -> + Response.Status.CONFLICT; + case PolicyMappingAlreadyExistsException policyMappingAlreadyExistsException -> + Response.Status.CONFLICT; + case PolicyInUseException policyInUseException -> Response.Status.BAD_REQUEST; + default -> Response.Status.INTERNAL_SERVER_ERROR; + }; } @Override diff --git a/service/common/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java b/service/common/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java index bc5f84c1d3..19d45c82ff 100644 --- a/service/common/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java +++ b/service/common/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java @@ -25,11 +25,14 @@ import ch.qos.logback.core.read.ListAppender; import com.fasterxml.jackson.core.JsonLocation; import com.fasterxml.jackson.core.JsonProcessingException; +import jakarta.ws.rs.core.Response; import jakarta.ws.rs.ext.ExceptionMapper; import java.util.Optional; import java.util.stream.Stream; import org.apache.polaris.core.exceptions.AlreadyExistsException; +import org.apache.polaris.core.exceptions.CommitConflictException; import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -74,6 +77,13 @@ public void testFullExceptionIsLogged( .isTrue(); } + @Test + public void testNamespaceException() { + PolarisExceptionMapper mapper = new PolarisExceptionMapper(); + Response response = mapper.toResponse(new CommitConflictException("test")); + Assertions.assertThat(response.getStatus()).isEqualTo(408); + } + static Stream testFullExceptionIsLogged() { // ConstraintViolationException isn't included because it doesn't propagate any info to its // inherited Exception From aa8a9b9f41e4ec06f113066ab2a04cb646929a0a Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Tue, 22 Jul 2025 14:15:10 +0100 Subject: [PATCH 6/7] removed no needed test --- .../catalog/AbstractIcebergCatalogTest.java | 27 ------------------- 1 file changed, 27 deletions(-) diff --git a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/AbstractIcebergCatalogTest.java b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/AbstractIcebergCatalogTest.java index 4d01d3817a..b4df56e8e9 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/AbstractIcebergCatalogTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/AbstractIcebergCatalogTest.java @@ -19,10 +19,8 @@ package org.apache.polaris.service.quarkus.catalog; import static java.nio.charset.StandardCharsets.UTF_8; -import static org.apache.polaris.core.persistence.dao.entity.BaseResult.ReturnStatus.ENTITY_NOT_FOUND; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Fail.fail; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.isA; import static org.mockito.Mockito.doReturn; @@ -446,31 +444,6 @@ protected boolean supportsNotifications() { return true; } - @Test - public void testPropertiesUpdateConcurrentException() { - PolarisMetaStoreManager spyMetaStore = spy(metaStoreManager); - PolarisPassthroughResolutionView passthroughView = - new PolarisPassthroughResolutionView( - polarisContext, entityManager, securityContext, CATALOG_NAME); - IcebergCatalog catalog = - new IcebergCatalog( - entityManager, - spyMetaStore, - polarisContext, - passthroughView, - securityContext, - Mockito.mock(TaskExecutor.class), - fileIOFactory, - polarisEventListener); - catalog.createNamespace(NS); - doReturn(new EntityResult(ENTITY_NOT_FOUND, "")) - .when(spyMetaStore) - .updateEntityPropertiesIfNotChanged(any(), any(), any()); - assertThrows( - CommitFailedException.class, () -> catalog.setProperties(NS, ImmutableMap.of("a", "b"))); - assertThrows(CommitFailedException.class, () -> catalog.removeProperties(NS, Set.of("a", "b"))); - } - @Test public void testEmptyNamespace() { IcebergCatalog catalog = catalog(); From 9e0bd3cfe9a5d2afaa756b1a29c824f0ec574da1 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Tue, 22 Jul 2025 16:35:40 +0100 Subject: [PATCH 7/7] updated response code --- .../polaris/service/exception/PolarisExceptionMapper.java | 2 +- .../apache/polaris/service/exception/ExceptionMapperTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java b/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java index 4832ee4e71..8b68d38901 100644 --- a/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java @@ -48,7 +48,7 @@ public class PolarisExceptionMapper implements ExceptionMapper private Response.Status getStatus(PolarisException exception) { return switch (exception) { case AlreadyExistsException alreadyExistsException -> Response.Status.CONFLICT; - case CommitConflictException commitConflictException -> Response.Status.REQUEST_TIMEOUT; + case CommitConflictException commitConflictException -> Response.Status.CONFLICT; case InvalidPolicyException invalidPolicyException -> Response.Status.BAD_REQUEST; case PolicyAttachException policyAttachException -> Response.Status.BAD_REQUEST; case NoSuchPolicyException noSuchPolicyException -> Response.Status.NOT_FOUND; diff --git a/service/common/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java b/service/common/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java index 19d45c82ff..6dc5f77d9c 100644 --- a/service/common/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java +++ b/service/common/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java @@ -81,7 +81,7 @@ public void testFullExceptionIsLogged( public void testNamespaceException() { PolarisExceptionMapper mapper = new PolarisExceptionMapper(); Response response = mapper.toResponse(new CommitConflictException("test")); - Assertions.assertThat(response.getStatus()).isEqualTo(408); + Assertions.assertThat(response.getStatus()).isEqualTo(409); } static Stream testFullExceptionIsLogged() {