Copilot commented on code in PR #9864:
URL: https://github.com/apache/gravitino/pull/9864#discussion_r2761851719
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/FunctionMetaService.java:
##########
@@ -110,6 +119,82 @@ public void insertFunction(FunctionEntity functionEntity,
boolean overwrite) thr
}
}
+ @Monitored(
+ metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
+ baseMetricName = "deleteFunction")
+ public boolean deleteFunction(NameIdentifier ident) {
+ FunctionPO functionPO = getFunctionPOByIdentifier(ident);
+ Long functionId = functionPO.functionId();
+
+ AtomicInteger functionDeletedCount = new AtomicInteger();
+ SessionUtils.doMultipleWithCommit(
+ // delete function meta
+ () ->
+ functionDeletedCount.set(
+ SessionUtils.getWithoutCommit(
+ FunctionMetaMapper.class,
+ mapper ->
mapper.softDeleteFunctionMetaByFunctionId(functionId))),
+
+ // delete function versions first
Review Comment:
The inline comment is misleading. The comment says "delete function versions
first" but the actual execution order is:
1. First, delete function meta (lines 131-136)
2. Then, delete function versions (lines 138-145)
The comment should be updated to accurately reflect that versions are
deleted second, not first. Consider changing it to "delete function versions"
or "delete function versions after meta deletion".
```suggestion
// delete function versions after meta deletion
```
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/FunctionMetaService.java:
##########
@@ -110,6 +119,82 @@ public void insertFunction(FunctionEntity functionEntity,
boolean overwrite) thr
}
}
+ @Monitored(
+ metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
+ baseMetricName = "deleteFunction")
+ public boolean deleteFunction(NameIdentifier ident) {
+ FunctionPO functionPO = getFunctionPOByIdentifier(ident);
+ Long functionId = functionPO.functionId();
+
+ AtomicInteger functionDeletedCount = new AtomicInteger();
+ SessionUtils.doMultipleWithCommit(
+ // delete function meta
+ () ->
+ functionDeletedCount.set(
+ SessionUtils.getWithoutCommit(
+ FunctionMetaMapper.class,
+ mapper ->
mapper.softDeleteFunctionMetaByFunctionId(functionId))),
+
+ // delete function versions first
+ () -> {
+ if (functionDeletedCount.get() > 0) {
+ SessionUtils.doWithoutCommit(
+ FunctionVersionMetaMapper.class,
+ mapper ->
mapper.softDeleteFunctionVersionsByFunctionId(functionId));
+ }
+ });
+
+ return functionDeletedCount.get() > 0;
+ }
+
+ @Monitored(
+ metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
+ baseMetricName = "deleteFunctionMetasByLegacyTimeline")
+ public int deleteFunctionMetasByLegacyTimeline(Long legacyTimeline, int
limit) {
+ int functionDeletedCount =
+ SessionUtils.doWithCommitAndFetchResult(
+ FunctionVersionMetaMapper.class,
+ mapper ->
mapper.deleteFunctionVersionMetasByLegacyTimeline(legacyTimeline, limit));
+
+ int functionVersionDeletedCount =
+ SessionUtils.doWithCommitAndFetchResult(
+ FunctionMetaMapper.class,
+ mapper ->
mapper.deleteFunctionMetasByLegacyTimeline(legacyTimeline, limit));
+
+ return functionDeletedCount + functionVersionDeletedCount;
Review Comment:
The variable names are swapped and do not match what they represent:
- `functionDeletedCount` (line 154) actually stores the count of deleted
function **versions** (from FunctionVersionMetaMapper)
- `functionVersionDeletedCount` (line 159) actually stores the count of
deleted function **meta** (from FunctionMetaMapper)
These should be renamed for clarity:
- Rename `functionDeletedCount` to `functionVersionDeletedCount`
- Rename `functionVersionDeletedCount` to `functionMetaDeletedCount`
This will make the code more readable and prevent confusion about what each
variable represents.
```suggestion
int functionVersionDeletedCount =
SessionUtils.doWithCommitAndFetchResult(
FunctionVersionMetaMapper.class,
mapper ->
mapper.deleteFunctionVersionMetasByLegacyTimeline(legacyTimeline, limit));
int functionMetaDeletedCount =
SessionUtils.doWithCommitAndFetchResult(
FunctionMetaMapper.class,
mapper ->
mapper.deleteFunctionMetasByLegacyTimeline(legacyTimeline, limit));
return functionVersionDeletedCount + functionMetaDeletedCount;
```
--
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]
