mchades commented on code in PR #14141:
URL: https://github.com/apache/gravitino/pull/14141#discussion_r4386149157
##########
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:
Changed reconciliation to compare exact metric name sets while retaining
duplicate-name validation. Regression coverage verifies skipped identity
access, persisted replacement definitions, version advancement, and stable IDs.
##########
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:
I'm keeping snapshot-to-snapshot backfill outside this PR. Semantic Model
storage is introduced in 2.0.0, so released-version upgrades have no existing
model data to migrate.
##########
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:
On JDK 17, the duplicate-key message already includes the metric name and
both `SemanticModelMetricPO` values, including `semanticModelId` and
`metricId`. Keeping the existing fail-fast behavior.
##########
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:
Added multi-row inserts in batches of 1,000 within the model transaction.
The ID generator is fetched and checked once, only when new IDs are needed.
Regression coverage includes batch boundaries, rollback, and reconciliation
without allocating new IDs.
--
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]
