jerryshao commented on code in PR #14040:
URL: https://github.com/apache/gravitino/pull/14040#discussion_r4333721143
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -161,10 +162,14 @@ public PolicyEntity alterPolicy(String metalake, String
policyName, PolicyChange
throw new NoSuchPolicyException(
"Policy with name %s under metalake %s does not exist", policyName,
metalake);
} catch (EntityAlreadyExistsException e) {
- throw new RuntimeException(
- String.format(
- "Trying to alter policy %s under metalake %s, but the new name
already exists",
- policyName, metalake));
+ String newName =
+ Arrays.stream(changes)
+ .filter(c -> c instanceof PolicyChange.RenamePolicy)
+ .map(c -> ((PolicyChange.RenamePolicy) c).getNewName())
+ .reduce((first, second) -> second)
+ .orElse(policyName);
+ throw new PolicyAlreadyExistsException(
+ e, "Policy with name %s under metalake %s already exists", newName,
metalake);
Review Comment:
[Important] This makes alterPolicy answer 409, but its OpenAPI operation
still documents only 200/404/5xx: docs/open-api/policies.yaml:135 (operationId
alterPolicy), responses at :142, has no "409" entry.
docs/open-api/models.yaml:116 (operationId updateModel) has the same gap. The
other four endpoints this PR changes already document it - filesets.yaml:108
(409 at :135), metalakes.yaml:101 (:120), catalogs.yaml:282 (:303),
tables.yaml:116 (:143) - so these two are the outliers, and both files already
define the example payload to point at (policies.yaml:1219
PolicyAlreadyExistsException, models.yaml:1068 ModelAlreadyExistsException).
Please add the two 409 blocks and re-run ./gradlew :docs:build.
Verified by: read the responses map of every alter/update operation in
docs/open-api/*.yaml on this checkout and grepped both files for the existing
AlreadyExists examples.
##########
core/src/main/java/org/apache/gravitino/catalog/ManagedTableOperations.java:
##########
@@ -181,8 +181,26 @@ public Table alterTable(NameIdentifier ident,
TableChange... changes)
} catch (NoSuchEntityException e) {
throw new NoSuchTableException(e, "Table %s does not exist", ident);
} catch (EntityAlreadyExistsException e) {
- throw new IllegalArgumentException(
- "Failed to rename table " + ident + " due to table already exists:
", e);
+ NameIdentifier newIdent =
+ Arrays.stream(changes)
+ .filter(c -> c instanceof TableChange.RenameTable)
+ .map(c -> (TableChange.RenameTable) c)
+ .map(
+ rename ->
+ NameIdentifier.of(
+ rename
+ .getNewSchemaName()
+ .map(
+ schema ->
+ NamespaceUtil.ofTable(
+ ident.namespace().level(0),
+ ident.namespace().level(1),
+ schema))
+ .orElse(ident.namespace()),
+ rename.getNewName()))
+ .reduce((first, second) -> second)
Review Comment:
[Nit] The conflicting identifier is derived a second time here, although the
cause already carries it: TableMetaService.updateTable builds its
EntityAlreadyExistsException from newTableEntity.nameIdentifier()
(core/src/main/java/org/apache/gravitino/storage/relational/service/TableMetaService.java:304),
i.e. the identifier applyChanges actually produced. This copy re-implements
applyChanges' namespace rule (ManagedTableOperations.java:248-254: the
namespace is recomputed from the stored entity on each RenameTable, so a
trailing name-only rename resets it to the original schema) and will drift
silently if that rule ever changes. Reusing the cause's identifier, or at least
a comment pointing at applyChanges, would keep the two in step.
Related question for you, which is also the substance of the two Copilot
findings: is that reset intended - should {rename("x", "other_schema"),
rename("y")} end up in other_schema rather than back in the original schema? If
the current semantics is intended, this derivation and the test expectation are
both correct as written and the Copilot comments can be dismissed.
Verified by: read applyChanges and TableMetaService.updateTable on this
checkout; the four change sequences in
core/src/test/java/org/apache/gravitino/storage/relational/TestRenameConflicts.java:193-199
all agree with the current reset behaviour.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -727,10 +726,15 @@ public Fileset alterFileset(NameIdentifier ident,
FilesetChange... changes)
throw ExceptionMessages.wrap("Failed to update fileset " + ident, ioe);
} catch (NoSuchEntityException nsee) {
throw new NoSuchFilesetException(nsee, FILESET_DOES_NOT_EXIST_MSG,
ident);
- } catch (AlreadyExistsException aee) {
- // This is happened when renaming a fileset to an existing fileset name.
- throw ExceptionMessages.wrap(
- "Fileset with the same name " + ident.name() + " already exists",
aee);
+ } catch (EntityAlreadyExistsException e) {
+ String newName =
+ Arrays.stream(changes)
+ .filter(c -> c instanceof FilesetChange.RenameFileset)
+ .map(c -> ((FilesetChange.RenameFileset) c).getNewName())
+ .reduce((first, second) -> second)
Review Comment:
[Nit] This filter/map/reduce block is now repeated almost verbatim in six
places: here (:730), ModelCatalogOperations.java:329, CatalogManager.java:1250,
MetalakeManager.java:346, PolicyManager.java:165 and
ManagedTableOperations.java:185. A small shared helper would be worth it for
more than the duplication - the pre-existing seventh copy, TagManager.java:169,
uses findFirst() instead of reduce((first, second) -> second), so a
multi-rename alterTag reports the first requested name rather than the one that
collided. The version in this PR is the correct one, since every updater
overwrites the name per change in order (FilesetCatalogOperations.java:1302,
TagManager.java:565), so folding TagManager onto the same helper would fix that
too - here or as a follow-up, your call.
Verified by: read all seven call sites and the updaters on this checkout.
--
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]
