jerryshao commented on PR #12602:
URL: https://github.com/apache/gravitino/pull/12602#issuecomment-5770992367
Verdict: blocking issues
java\n case SEMANTIC_MODEL:\n // TODO: Delegate to
SemanticModelMetaService when relational persistence is added.\n return
0;\n```\n\nThis PR *is* that delegation target: before it, no snapshot rows
existed, so the stub was harmless. Now a repeated `put(entity, true)` — a
pipeline redeploying the same definition, say — grows the table without bound.
`TestSemanticModelJDBCBackend.java:369` asserts exactly this retain-everything
behaviour (`List.of(1, 2, 3)`) with nothing configured to trim it.\n\nTwo
established shapes to pick from: `ViewMetaService` retires the superseded
snapshot inline, and `FunctionMetaService` retains snapshots but backs that
with `deleteFunctionVersionsByRetentionCount` wired into
`deleteOldVersionData`. This PR takes Function's retention model without
Function's collector, which is the one combination that leaks. Adding
`deleteSemanticModelVersionsByRetentionCount` and replacing the stub would
close it.\n\nVerified by: read `JDBCBacken
d.java:541-585` and `SemanticModelMetaService.java:146-151` in the checked-out
PR tree; `grep -rn \"deleteFunctionVersionsByRetentionCount\"` confirms the
Function precedent, and `TestSemanticModelJDBCBackend.java:369` confirms
snapshots accumulate."
},
{
"path":
"core/src/main/java/org/apache/gravitino/storage/relational/service/SemanticModelMetaService.java",
"start_line": 139,
"line": 143,
"side": "RIGHT",
"severity": "Nit",
"body": "[Nit] A lost overwrite here throws `IllegalStateException`,
which no caller can classify.\n\n`ExceptionUtils.checkSQLException` (line 153)
only converts when `re.getCause() instanceof SQLException`
(`ExceptionUtils.java:32-38`), so a `Preconditions.checkState` failure falls
straight through the `catch` and out of `insertSemanticModel` as a raw
`IllegalStateException` — not the `OptimisticLockException` that retry logic
keys on, and not `NoSuchEntityException`. The sibling path throws the
classified exception instead: `ViewMetaService.java:317-318` does `if (updated
== 0) throw viewWriteFailure(...)`, which routes through
`OccWriteSupport.writeFailure` and picks `NoSuchEntityException` or
`ExceptionUtils.concurrentModification` based on a re-read.\n\nI could not
construct a reachable case: the row is held by `FOR UPDATE` for the whole
transaction and `nextVersion` is always above every existing snapshot version,
so `updated` should be 1. That makes this defensive r
ather than a live bug — but if the guard is worth keeping, it is worth
throwing what the rest of the module throws.\n\nVerified by: read
`SemanticModelMetaService.java:136-155`, `ExceptionUtils.java:32-38`,
`ViewMetaService.java:314-322` and `OccWriteSupport.java:113-135` in the
checked-out PR tree."
},
{
"path":
"core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/SemanticModelMetaBaseSQLProvider.java",
"start_line": 183,
"line": 195,
"side": "RIGHT",
"severity": "Nit",
"body": "[Nit] This upsert is now unreachable, and it is the same SQL
flagged last round as a version-bump
trap.\n\n`insertSemanticModelMetaOnDuplicateKeyUpdate` is only reached from
`SemanticModelPOStorageOps.insertPO(..., overwrite=true)`, and the one call
site passes a hard-coded `false` (`SemanticModelMetaService.java:133`). So the
`last_version = current_version + 1` / `current_version = current_version + 1`
pair on lines 193-194, plus the `SemanticModelMetaPostgreSQLProvider` override,
now computes a version bump that nothing executes — while the Java path
computes its own `nextVersion` at `SemanticModelMetaService.java:166-171`. Two
copies of the same rule, one of them dead, is exactly the drift the last
round's finding was
about.\n\n`SemanticModelVersionInfoMapper.insertSemanticModelVersionInfoOnDuplicateKeyUpdate`
and its two providers have no callers at all, and
`selectSemanticModelMetaByIdForUpdate` is the third (see the separate comment
about the missing by-ID
lock, which is the one of the three that should arguably be wired rather than
deleted).\n\nVerified by: `grep -rn` over `core/src` for
`insertSemanticModelMetaOnDuplicateKeyUpdate`,
`insertSemanticModelVersionInfoOnDuplicateKeyUpdate` and
`selectSemanticModelMetaByIdForUpdate` — the first reaches only
`SemanticModelPOStorageOps.java:40`, and `SemanticModelMetaService.java:133` is
the sole `insertPO` caller, passing `false`."
}
]
}
```
---
_Generated by [Claude Code](https://claude.ai/code)_
--
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]
