jerryshao commented on code in PR #14336:
URL: https://github.com/apache/gravitino/pull/14336#discussion_r4409591893
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -404,8 +412,10 @@ public Table alterTable(NameIdentifier ident,
TableChange... changes)
ident, newIdent);
}
- return EntityCombinedTable.of(alteredTable, updatedTableEntity)
- .withHiddenProperties(catalogResult.hiddenProperties);
+ return checkColumnsReported(
+ ident,
+ EntityCombinedTable.of(alteredTable, updatedTableEntity)
+ .withHiddenProperties(catalogResult.hiddenProperties));
Review Comment:
[Question] For a rename, throwing here is a narrow behaviour change from
`af25aa4`'s predecessor that the code comment does not quite name: the
authorization plugin is now left holding the old name permanently.
`TableHookDispatcher.alterTable` calls
`AuthorizationUtils.authorizationPluginRenamePrivileges` only after
`dispatcher.alterTable(...)` returns (`TableHookDispatcher.java:121-133`), and
the hook sits inside the event dispatcher (`GravitinoEnv.java:1108-1114`). So
on this path:
- before this PR: the store update ran, every stored column was treated as
dropped, the hook's rename ran, and the NPE then came out of `new
TableInfo(table)` - the privileges followed the new name, the columns were
destroyed;
- after this PR: the columns survive (the real fix), but the throw happens
before the hook, so the catalog and the Gravitino store carry the new name
while the authorization plugin still has the old one. A retry cannot repair it,
because the catalog rename already happened, as your comment notes.
Trading a full column wipe for a stale name in the plugin is clearly the
better trade, and this needs a catalog that reports no column list, so I would
not hold the PR for it. But it is cheap to close, either by doing the rename
before throwing, or by null-guarding `TableInfo.columns` (as suggested on the
first pass) so the alter can complete with the stored columns and only log the
missing list. Which would you prefer?
Verified by: reading `TableHookDispatcher.java:104-136`,
`GravitinoEnv.java:1108-1114`, `TableInfo.java:93-100`, and the pre-PR
`updateColumnsIfNecessary` behaviour in `git show
origin/main:core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java`
on this branch.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/JobMetaService.java:
##########
@@ -256,7 +258,13 @@ void deleteJobWithVersion(NameIdentifier ident, JobPO
observed) {
mapper ->
mapper.softDeleteJobByRunIdWithVersion(
observed.jobRunId(),
observed.currentVersion())),
- () -> writeFailure(ident, observed)));
+ () -> writeFailure(ident, observed)),
+ () ->
+ SessionUtils.doWithoutCommit(
+ OwnerMetaMapper.class,
+ mapper ->
+ mapper.softDeleteOwnerRelByMetadataObjectIdAndType(
+ observed.jobRunId(), MetadataObject.Type.JOB.name())));
Review Comment:
[Question] Should `deleteJobsByLegacyTimeline` get the same cleanup? It is
the path that reaps most jobs, and it still leaves the owner rows to the orphan
GC.
`deleteJobsByLegacyTimeline` (`JobMetaService.java:178-186`) soft-deletes
every finished job older than the retention window and then hard-deletes the
rows already past it, and it touches no `OwnerMetaMapper` at all. So an
explicitly deleted job now loses its owner row in the same transaction, while a
job that simply ages out keeps it live until
`OrphanedMetadataObjectRelationService` runs - which is exactly the window this
PR set out to close for the explicit path.
It is not a leak: that service does map `MetadataObject.Type.JOB` to
`job_run_meta.job_run_id`, so the rows are collected eventually, including
after the hard delete. And a job's owner is a less visible artefact than a
role's privilege on a dead template. So this is a scope question, not a defect:
is leaving the retention path to the orphan GC deliberate, or worth one more
`softDeleteOwnerRelByMetadataObjectIdAndType` by timeline?
Verified by: reading `JobMetaService.java:174-187`,
`JobMetaBaseSQLProvider.java:173-186` for both legacy-timeline statements, and
the type map in `OrphanedMetadataObjectRelationService.java:70-85` on this
branch.
##########
common/src/main/java/org/apache/gravitino/dto/rel/TableDTO.java:
##########
@@ -309,7 +309,7 @@ public S withIndex(IndexDTO[] indexes) {
* Builds a Table DTO based on the provided builder parameters.
*
* @return A new TableDTO instance.
- * @throws IllegalArgumentException If required fields name, columns and
audit are not set.
+ * @throws IllegalArgumentException If required fields name and audit are
not set.
Review Comment:
[Nit] This widens a public contract to serve one internal hop. Worth marking
the field rather than only relaxing the text.
With the javadoc now saying only name and audit are required, a null
`columns` is documented as legal for every `TableDTO` consumer, server and
client alike - but `TableDTO.columns()` (`TableDTO.java:112-113`) carries no
`@Nullable`, and the only producer that is meant to use the allowance is
`ConnectorObjectSnapshot.detach` (`ConnectorObjectSnapshot.java:43-44`). The
safety of everything downstream rests on the dispatcher never emitting such a
DTO, which is invisible from here.
Adding `@Nullable` to the field and the `columns()` accessor, and a word in
the `withColumns` javadoc that null is only for the internal connector
snapshot, would keep the next reader from treating it as a supported wire state.
Verified by: reading `TableDTO.java:40-50`, `:105-115`, `:230-240`,
`:305-325` and the single null-passing caller in `DTOConverters.java:296-315`
on this branch.
--
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]
