jerryshao commented on code in PR #14141:
URL: https://github.com/apache/gravitino/pull/14141#discussion_r4385960349
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SemanticModelMetaService.java:
##########
@@ -385,11 +407,65 @@ void deleteSemanticModelWithVersion(
SessionUtils.doWithoutCommit(
SemanticModelVersionInfoMapper.class,
mapper ->
mapper.softDeleteSemanticModelVersionsBySemanticModelId(semanticModelId)),
+ () ->
+ SessionUtils.doWithoutCommit(
+ SemanticModelMetricMapper.class,
+ mapper ->
mapper.softDeleteSemanticModelMetricsBySemanticModelId(semanticModelId)),
() ->
EntityChangeLogWriter.append(
semanticModelIdent, Entity.EntityType.SEMANTIC_MODEL,
OperateType.DROP));
}
+ private static void reconcileMetricIdentities(
+ SemanticModelPO model, SemanticModelDefinition definition) {
+ Set<String> names = new LinkedHashSet<>();
+ Metric[] metrics = definition.metrics();
+ if (metrics != null) {
+ for (Metric metric : metrics) {
+ Preconditions.checkArgument(
+ names.add(metric.name()), "Duplicate metric name: %s",
metric.name());
+ }
+ }
+
+ // The caller has already inserted or version-checked the model in this
transaction. A locking
+ // read also observes the latest identities on backends using
repeatable-read isolation.
+ SessionUtils.doWithoutCommit(
+ SemanticModelMetricMapper.class,
+ mapper -> {
+ Map<String, SemanticModelMetricPO> live =
+ mapper
+
.selectSemanticModelMetricsByModelIdForUpdate(model.getSemanticModelId())
+ .stream()
+ .collect(
+ Collectors.toMap(SemanticModelMetricPO::metricName,
Function.identity()));
Review Comment:
[Nit] `Collectors.toMap` throws `IllegalStateException: Duplicate key ...`
if two live rows ever share a name for one model. Per the design that cannot
happen - uniqueness is enforced by validation plus model concurrency control,
deliberately `without a database name-uniqueness index` - which also means this
map is the only place the invariant is ever observed, and one anomalous row
would make every subsequent create, alter and overwrite of that model fail with
a message naming neither the model nor the metric. A `toMap(..., ..., (a, b) ->
{ throw new IllegalStateException(...) })` carrying `semanticModelId` and the
metric name (or keeping the lower ID and retiring the other) would turn an
opaque failure into a diagnosable one at no runtime cost.
Verified by: read lines 433-440; confirmed
`scripts/mysql/schema-2.0.0-mysql.sql` defines only `KEY idx_smmm_model_live
(semantic_model_id, deleted_at)` for this table, unlike `uk_sid_smn_del` on
`semantic_model_meta` (line 577), and that
`design-docs/gravitino-schema-metrics-discovery-design.md:319-321` makes that
omission intentional.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SemanticModelMetaService.java:
##########
@@ -250,6 +260,13 @@ public <E extends Entity & HasIdentifier>
SemanticModelEntity updateSemanticMode
throw semanticModelWriteFailure(identifier, oldSemanticModelPO);
}
},
+ () -> {
+ if (!Objects.equals(
+ oldSemanticModelPO.getSemanticModelVersionInfoPO().metrics(),
+ newSemanticModelPO.getSemanticModelVersionInfoPO().metrics()))
{
+ reconcileMetricIdentities(newSemanticModelPO,
newEntity.definition());
Review Comment:
[Question] Because this is the only reconciliation trigger on the alter
path, a model that has no identity rows never acquires them: as long as its
`metrics` value stays byte-identical, reconciliation is skipped forever, and
the upgrade scripts only `CREATE TABLE` without a backfill. For released
upgrades that is a non-issue - `design-docs/gravitino-semantic-model-design.md`
states that Semantic Model tables are introduced in 2.0.0 and that released
versions have no Semantic Model data to migrate - but a database written by an
earlier 2.0.0-SNAPSHOT build will hold Semantic Models with metrics and no rows
in the new table, and they would stay invisible to the schema-level discovery
built on top of this.
Is a self-heal worth it (for example, also reconcile when the live set is
empty while the new definition has names), or is pre-release in-place schema
churn explicitly out of scope here?
Verified by: grepped every call site of `reconcileMetricIdentities` (only
lines 185 and 267) and read all three `upgrade-1.3.0-to-2.0.0-*.sql` diffs,
which add the table and nothing else.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SemanticModelMetaService.java:
##########
@@ -250,6 +260,13 @@ public <E extends Entity & HasIdentifier>
SemanticModelEntity updateSemanticMode
throw semanticModelWriteFailure(identifier, oldSemanticModelPO);
}
},
+ () -> {
+ if (!Objects.equals(
+ oldSemanticModelPO.getSemanticModelVersionInfoPO().metrics(),
+ newSemanticModelPO.getSemanticModelVersionInfoPO().metrics()))
{
Review Comment:
[Nit] Comparing the serialized `metrics` JSON means any content-only edit
(description, expression, metric order) also triggers reconciliation, and
`reconcileMetricIdentities` then issues `SELECT ... FOR UPDATE` over every live
identity row of the model only to write nothing. Only the *name set* can change
identities, and both definitions are in scope here, so
`oldSemanticModelEntity.definition().metrics()` vs
`newEntity.definition().metrics()` compared as name sets would skip the locking
read for the common content-only alter and keep today's behaviour for every
name change.
Verified by: read `updateSemanticModel` (lines 215-281) and
`reconcileMetricIdentities` (lines 419-468) in full on 6ec0cde - the reconcile
body always runs the locking select before deciding there is nothing to insert
or retire; `testExactNamesContentChangesAndReintroduction` asserts exactly this
no-op case (same four names, new expression and description, IDs unchanged).
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SemanticModelMetaService.java:
##########
@@ -385,11 +407,65 @@ void deleteSemanticModelWithVersion(
SessionUtils.doWithoutCommit(
SemanticModelVersionInfoMapper.class,
mapper ->
mapper.softDeleteSemanticModelVersionsBySemanticModelId(semanticModelId)),
+ () ->
+ SessionUtils.doWithoutCommit(
+ SemanticModelMetricMapper.class,
+ mapper ->
mapper.softDeleteSemanticModelMetricsBySemanticModelId(semanticModelId)),
() ->
EntityChangeLogWriter.append(
semanticModelIdent, Entity.EntityType.SEMANTIC_MODEL,
OperateType.DROP));
}
+ private static void reconcileMetricIdentities(
+ SemanticModelPO model, SemanticModelDefinition definition) {
+ Set<String> names = new LinkedHashSet<>();
+ Metric[] metrics = definition.metrics();
+ if (metrics != null) {
+ for (Metric metric : metrics) {
+ Preconditions.checkArgument(
+ names.add(metric.name()), "Duplicate metric name: %s",
metric.name());
+ }
+ }
+
+ // The caller has already inserted or version-checked the model in this
transaction. A locking
+ // read also observes the latest identities on backends using
repeatable-read isolation.
+ SessionUtils.doWithoutCommit(
+ SemanticModelMetricMapper.class,
+ mapper -> {
+ Map<String, SemanticModelMetricPO> live =
+ mapper
+
.selectSemanticModelMetricsByModelIdForUpdate(model.getSemanticModelId())
+ .stream()
+ .collect(
+ Collectors.toMap(SemanticModelMetricPO::metricName,
Function.identity()));
+ for (String name : names) {
+ if (live.remove(name) != null) {
+ continue;
+ }
+ IdGenerator generator = GravitinoEnv.getInstance().idGenerator();
Review Comment:
[Nit] Two small things in this loop: the
`GravitinoEnv.getInstance().idGenerator()` lookup and its `checkState` repeat
once per new metric and can be hoisted above the loop, and each identity is a
separate round trip. Gravitino's precedent for inserting a model's child rows
is one multi-row statement -
`TableColumnMapper.insertColumnPOs(List<ColumnPO>)` backed by a `<foreach>`
VALUES list
(`core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/TableColumnBaseSQLProvider.java:61`).
An `insertSemanticModelMetrics(List<SemanticModelMetricPO>)` provider would
make a create with N metrics one INSERT instead of N, inside the same
transaction. Not blocking: metric counts per model are presumably small.
Verified by: read the loop at lines 441-458 and the column batch-insert
mapper/provider it would mirror (`TableColumnMapper.java:39-40`).
--
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]
