imbajin commented on PR #2937:
URL:
https://github.com/apache/incubator-hugegraph/pull/2937#issuecomment-3758368354
## 代码审查总结
感谢提交这个重要的架构简化 PR!我已经详细审查了所有变更,除了已经在具体代码行发布的评论外,还有以下**关键问题**需要关注:
---
### ‼️ **高优先级问题**
#### 1. Server ID 自动生成机制存在缺陷
在 `GraphManager` 构造函数中:
```java
server = "server-" + UUID.randomUUID().toString().substring(0, 8);
```
**问题**:
- 每次重启会生成新的 ID,导致数据库残留旧的 ServerInfo 记录
- 截取 UUID 前 8 位增加碰撞风险
- 生成的 ID 未持久化,难以追踪和调试
**建议**: 使用 `hostname + processId` 生成稳定 ID,或将 UUID 长度增加到 16 位。
---
#### 2. 调度器类型推断逻辑过于简化
在 `StandardHugeGraph.schedulerType()`:
```java
return StandardHugeGraph.this.isHstore() ? "distributed" : "local";
```
**问题**:
- 仅根据 backend 类型判断,无法覆盖 RocksDB 分布式部署场景
- 未检查 `pd.peers` 等分布式配置
**建议**: 结合 PD 配置或增加显式配置项来判断调度模式。
---
#### 3. 任务删除逻辑发生重大变更
`DistributedTaskScheduler.delete()` 移除了 DELETING 状态的设置逻辑:
**原逻辑**: `force=false` 设置为 DELETING 状态,由定时任务清理
**新逻辑**: `force=false` 时直接删除完成的任务
**影响**: 可能破坏依赖定时清理的代码路径,需在 PR 描述中明确说明此行为变更。
---
### ⚠️ **中等优先级问题**
#### 4. 线程安全隐患
`DistributedTaskScheduler.cancel()` 方法缺少对 `runningTasks`
并发访问的同步保护,可能导致任务在检查和取消之间状态变化。
---
#### 5. 异常处理不完整
`TaskAndResultScheduler.taskWithoutResult()` 新增了
`NotFoundException`,但调用方未统一处理该异常,可能导致不一致的错误行为。
---
#### 6. Ephemeral Job 行为变更未说明
测试用例 `testEphemeralJob()` 的修改表明 Ephemeral Job 不再持久化到数据库,这是重大的 API 行为变更,需要:
- 在 PR 描述中明确说明
- 更新用户文档
- 提供迁移指南
---
#### 7. 资源关闭顺序问题
`DistributedTaskScheduler.close()` 中捕获了 `InterruptedException` 但未恢复线程中断状态:
```java
} catch (ExecutionException | InterruptedException e) {
LOG.warn("Exception while waiting for cron task to complete", e);
// 应该添加: if (e instanceof InterruptedException)
Thread.currentThread().interrupt();
}
```
---
### 🧹 **代码质量建议**
1. **TODO 注释应更明确**: 第 334 行的 TODO 缺少问题编号和时间节点
2. **测试用例修改需要说明**: `MultiGraphsTest.testCreateGraphsWithSameName()`
修改图名称的原因未说明
3. **代码覆盖率下降**: Codecov 报告显示 `DistributedTaskScheduler` 新增代码覆盖率仅 0%
---
### 📋 **建议的后续行动**
1. 为 Server ID 生成添加稳定性保证机制
2. 改进调度器类型推断逻辑,支持更多部署场景
3. 明确 API 行为变更并更新文档
4. 增加新增代码路径的单元测试覆盖
5. 考虑添加配置迁移指南
总体而言,这是一个很好的架构简化尝试,但需要解决上述关键问题以确保生产环境的稳定性和可维护性。
--
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]
