imbajin commented on code in PR #3060:
URL: https://github.com/apache/hugegraph/pull/3060#discussion_r3447643828
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/DistributedTaskScheduler.java:
##########
@@ -306,10 +306,13 @@ protected <V> HugeTask<V> deleteFromDB(Id id) {
Iterator<Vertex> vertices = this.tx().queryTaskInfos(id);
HugeVertex vertex = (HugeVertex) QueryResults.one(vertices);
if (vertex == null) {
+ this.deleteTaskResultFromTx(id);
return null;
}
- HugeTask<V> result = HugeTask.fromVertex(vertex);
- this.tx().removeVertex(vertex);
+ HugeTask<V> result = HugeTask.fromVertex(vertex, false);
+ // Prefer a retryable orphan result over a live task missing
result.
+ this.tx().removeTaskVertex(vertex);
Review Comment:
‼️ **Make orphan result cleanup reachable from production retries**
This failure mode is now explicitly tested, but the recovery path in the
test calls `deleteFromDBForTest(id)` directly after the task vertex has already
been removed. In production, `cronSchedule()` only discovers delete work by
scanning `DELETING` task vertices, while `TaskAndResultTransaction` is
auto-commit and `removeTaskVertex()` commits through `afterWrite()`. If
`deleteTaskResultFromTx()` fails after this line, the remaining `~taskresult`
vertex has no task vertex/status index entry left for the scheduler to
rediscover, so the large result can leak permanently. Please either keep a
retryable tombstone until result cleanup succeeds, add a production
orphan-result cleanup path, or keep the task vertex until the result delete has
succeeded; the regression test should exercise that production recovery path
rather than only a direct helper call.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/TaskAndResultScheduler.java:
##########
@@ -103,41 +103,65 @@ public <V> void save(HugeTask<V> task) {
}
@Override
- public <V> HugeTask<V> task(Id id) {
+ public <V> HugeTask<V> task(Id id, boolean withResult) {
HugeTask<V> task = this.call(() -> {
Iterator<Vertex> vertices = this.tx().queryTaskInfos(id);
Vertex vertex = QueryResults.one(vertices);
if (vertex == null) {
return null;
}
- return HugeTask.fromVertex(vertex);
+ return HugeTask.fromVertex(vertex, withResult);
});
if (task == null) {
throw new NotFoundException("Can't find task with id '%s'", id);
}
- HugeTaskResult taskResult = queryTaskResult(id);
- if (taskResult != null) {
- task.result(taskResult);
+ if (withResult) {
+ HugeTaskResult taskResult = queryTaskResult(id);
+ if (taskResult != null) {
+ task.result(taskResult);
+ }
}
return task;
}
@Override
public <V> Iterator<HugeTask<V>> tasks(List<Id> ids) {
- return this.tasksWithoutResult(ids);
+ return this.tasks(ids, true);
Review Comment:
⚠️ **Preserve metadata-only defaults for bulk task scans**
Before this PR, the task/result-backed scheduler's no-flag `tasks(ids)` and
`tasks(status, limit, page)` overloads returned metadata-only tasks; the
result-loading path was only in the protected `queryTask(...)` helpers. This
now routes legacy bulk callers through `withResult=true`, which means any
caller not audited in this PR can again load/decompress historical huge task
results. Please keep the no-flag bulk overloads metadata-only for this
scheduler and require callers that need payloads to opt in through the new
`withResult=true` overloads; add a regression covering the no-flag bulk
behavior so this boundary stays explicit.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/task/TaskScheduler.java:
##########
@@ -47,12 +47,25 @@ public interface TaskScheduler {
<V> HugeTask<V> delete(Id id, boolean force);
- <V> HugeTask<V> task(Id id);
+ default <V> HugeTask<V> task(Id id) {
+ return this.task(id, true);
+ }
- <V> Iterator<HugeTask<V>> tasks(List<Id> ids);
+ <V> HugeTask<V> task(Id id, boolean withResult);
Review Comment:
⚠️ **Keep existing TaskScheduler implementers source-compatible**
The old no-argument methods were changed to defaults, but the new
`withResult` overloads are abstract. Any existing downstream `TaskScheduler`
implementation that compiled against the old interface now has to add three new
methods before it can recompile, even if it does not support separated task
results. Please make the new overloads default methods that delegate to the old
methods, and let only the built-in schedulers override them where
`withResult=false` is meaningful.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
