jerryshao commented on code in PR #13487:
URL: https://github.com/apache/gravitino/pull/13487#discussion_r4229968962
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/StatisticMetaService.java:
##########
@@ -63,10 +77,23 @@ public List<StatisticEntity> listStatisticsByEntity(
.collect(Collectors.toList());
}
+ /**
+ * Creates or replaces statistics of a metadata object by name.
+ *
+ * <p>Existing statistics are replaced only if their version is unchanged
since this call read
+ * them, and missing statistics are inserted only if nobody created them
meanwhile; both cases
+ * otherwise fail the whole batch with {@link OptimisticLockException}. The
target is fenced in
+ * the same transaction, so a target dropped or replaced after its ID was
resolved fails with
+ * {@link NoSuchEntityException}.
+ *
+ * @param statisticEntities the statistics to write; names must be unique in
the batch
+ * @param entity the metadata object that owns the statistics
+ * @param type the metadata object type
+ */
@Monitored(
metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "batchInsertStatisticPOsOnDuplicateKeyUpdate")
- public void batchInsertStatisticPOsOnDuplicateKeyUpdate(
+ public void writeStatisticsWithVersion(
Review Comment:
[Nit] Now that the method is renamed, the `baseMetricName` literal two lines
up ("batchInsertStatisticPOsOnDuplicateKeyUpdate") deliberately no longer
matches the method name - that divergence is what keeps the emitted metric
stable across the rename, and it is the only remaining occurrence of the old
name in the repository. Nothing in the file says so, so a later cleanup that
drops the literal or "fixes" the mismatch would silently rename a metric that
dashboards may key on. One short comment on the annotation would prevent that.
Verified by: `grep -rn batchInsertStatisticPOsOnDuplicateKeyUpdate` over the
checkout matches only StatisticMetaService.java:95; the rename itself is
correct and fully propagated.
##########
api/src/main/java/org/apache/gravitino/stats/SupportsStatistics.java:
##########
@@ -53,9 +61,16 @@ void updateStatistics(Map<String, StatisticValue<?>>
statistics)
* Drop statistics by their names. If the statistic is unmodifiable, it will
throw an
* UnmodifiableStatisticException.
*
+ * <p>For Gravitino-managed table statistics, a concurrent modification or
same-name replacement
+ * can fail the entire drop with {@link OptimisticLockException}. No part of
that drop is
+ * committed; retry the operation against the current state. A statistic
already removed by
+ * another drop is not a conflict and is not counted as dropped by this call.
Review Comment:
[Nit] `updateStatistics` was reworded in 138b3a3 to "The caller decides
whether to abort or retry against the current state; Gravitino does not
automatically retry the write", but this paragraph kept the earlier imperative
"retry the operation against the current state" from d3c21db. The same
interface now gives two different recovery contracts for the same OCC outcome,
and the drop wording is the one the PR deliberately moved away from. Suggest
matching the `updateStatistics` phrasing here (the `@throws` tag below is
already consistent).
Verified by: comparing lines 48-52 with 64-67 in the file at this head, and
`git show 138b3a3 --
api/src/main/java/org/apache/gravitino/stats/SupportsStatistics.java`, which
only rewrites the update paragraph.
--
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]
