dennishuo commented on code in PR #243:
URL: https://github.com/apache/polaris/pull/243#discussion_r1746609767
##########
extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java:
##########
@@ -257,6 +259,12 @@ public <T> T runInTransaction(
} finally {
localSession.remove();
}
+ } catch (PersistenceException e) {
+ if (e.toString().toLowerCase(Locale.ROOT).contains("duplicate key")) {
+ throw new IllegalStateException("Duplicate key error when persisting
entity", e);
Review Comment:
It looks like we do already have some specializations for specific jakarta
exceptions in
[IcebergExceptionMapper](https://github.com/apache/polaris/blob/8f14363abc42ea6d95b67bc407f825b0af9bb4f6/polaris-service/src/main/java/org/apache/polaris/service/IcebergExceptionMapper.java#L71)
such as for:
case jakarta.ws.rs.ForbiddenException e ->
Response.Status.FORBIDDEN.getStatusCode();
There's also a kind of catch-all for:
case WebApplicationException e -> e.getResponse().getStatus();
To avoid throwing a 500 `INTERNAL_SERVER_ERROR` maybe we can either throw
`jakarta.persistence.EntityExistsException` here and add a case there to
translate that into `CONFLICT` or we could throw a generic
`WebApplicationException` here with `CONFLICT` status code already set.
##########
polaris-service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java:
##########
@@ -505,20 +515,23 @@ public Response addGrantToCatalogRole(
catalogName, catalogRoleName, Namespace.of(namespaceParts),
privilege);
break;
}
- case CatalogGrant catalogGrant:
+ case CatalogGrant catalogGrant:
{
PolarisPrivilege privilege =
PolarisPrivilege.valueOf(catalogGrant.getPrivilege().toString());
adminService.grantPrivilegeOnCatalogToRole(catalogName,
catalogRoleName, privilege);
break;
}
- default:
- LOGGER
- .atWarn()
- .addKeyValue("catalog", catalogName)
- .addKeyValue("role", catalogRoleName)
- .log("Don't know how to handle privilege grant: {}", grantRequest);
- return Response.status(Response.Status.BAD_REQUEST).build();
+ default:
+ LOGGER
+ .atWarn()
+ .addKeyValue("catalog", catalogName)
+ .addKeyValue("role", catalogRoleName)
+ .log("Don't know how to handle privilege grant: {}",
grantRequest);
+ return Response.status(Response.Status.BAD_REQUEST).build();
+ }
+ } catch (IllegalStateException e) {
+ throw new AlreadyExistsException("Grant already exists or resolution
failed");
Review Comment:
If we throw `jakarta.persistence.EntityExistsException` instead we won't
need to assume that `IlegalStateException` should translate to
`AlreadyExistsException`. In particular, some elements of the MetaStoreManger
very well may throw `IllegalStateException` for true illegal states that are
server errors and wouldn't make sense to translate into
`AlreadyExistsException`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
