imbajin commented on code in PR #687:
URL:
https://github.com/apache/incubator-hugegraph-toolchain/pull/687#discussion_r2480146657
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/HugeGraphLoader.java:
##########
@@ -113,7 +113,6 @@ public static void main(String[] args) {
loader = new HugeGraphLoader(args);
} catch (Throwable e) {
Printer.printError("Failed to start loading", e);
- System.exit(1);
return;
Review Comment:
‼️ **Critical: 错误移除了必要的 System.exit(1) 调用**
原代码在启动失败时调用 `System.exit(1)` 是正确的设计,因为:
1. 这是一个主程序入口点,启动失败应该终止进程并返回错误码
2. CI/CD 和脚本依赖退出码来判断加载是否成功
3. 移除后,即使启动失败程序也会正常退出(退出码 0),导致调用方误以为成功
建议恢复原有逻辑。
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graphs/GraphsAPI.java:
##########
@@ -136,14 +136,8 @@ public void clear(String graph, boolean clearSchema) {
}
public void clear(String graph, String message) {
- clear(null, graph, message);
- }
-
- public void clear(String graphSpace, String graph, String message) {
- String path = (graphSpace == null)
- ? joinPath(this.path(), graph, CLEAR)
- : joinPath(this.path(), graphSpace, graph, CLEAR);
- this.client.delete(path, ImmutableMap.of(CONFIRM_MESSAGE, message));
+ this.client.delete(joinPath(this.path(), graph, CLEAR),
Review Comment:
‼️ **Critical: GraphSpace 相关功能被完全删除**
这个 PR 删除了所有 graphSpace 相关的方法重载:
- `clear(String graphSpace, String graph, String message)`
- `mode(String graphSpace, String graph, GraphMode mode)`
- `readMode(String graphSpace, String graph, GraphReadMode readMode)`
**问题:**
1. 如果 HugeGraph 1.7.0 仍然支持 graph space 特性,这是一个破坏性变更
2. 没有迁移指南或废弃通知
3. 可能导致依赖这些 API 的用户代码编译失败
**需要确认:**
- HugeGraph 1.7.0 是否完全移除了 graph space 特性?
- 如果是,需要在 PR 描述中说明并提供迁移指南
- 如果不是,这些方法不应该被删除
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graphs/GraphsAPI.java:
##########
@@ -204,85 +198,51 @@ public Map<String, String> reload() {
}
public void mode(String graph, GraphMode mode) {
- mode(null, graph, mode);
- }
-
- public void mode(String graphSpace, String graph, GraphMode mode) {
// NOTE: Must provide id for PUT. If you use "graph/mode", "/" will
// be encoded to "%2F". So use "mode" here, although inaccurate.
- if (graphSpace == null) {
- this.client.put(joinPath(this.path(), graph, MODE), null, mode);
- return;
- }
- this.client.put(joinPath(this.path(), graphSpace, graph, MODE), null,
mode);
+ this.client.put(joinPath(this.path(), graph, MODE), null, mode);
}
Review Comment:
⚠️ **代码重复: 缺少 DRY 原则**
`mode()` 和 `readMode()` 方法中有大量重复代码,包括:
- 相同的 REST 调用模式
- 相同的错误处理逻辑
- 相同的枚举值解析
建议提取为通用方法以提高可维护性:
```suggestion
private <T extends Enum<T>> T getMode(String graph, String modeKey, Class<T>
enumClass,
String displayName) {
RestResult result = this.client.get(joinPath(this.path(), graph),
modeKey);
@SuppressWarnings("unchecked")
Map<String, String> map = result.readObject(Map.class);
String value = map.get(modeKey);
if (value == null) {
throw new InvalidResponseException(
"Invalid response, expect '%s' in response", modeKey);
}
try {
return Enum.valueOf(enumClass, value);
} catch (IllegalArgumentException e) {
throw new InvalidResponseException(
"Invalid %s value '%s'", displayName, value);
}
}
public GraphMode mode(String graph) {
return getMode(graph, MODE, GraphMode.class, "GraphMode");
}
public GraphReadMode readMode(String graph) {
this.client.checkApiVersion("0.59", "graph read mode");
return getMode(graph, GRAPH_READ_MODE, GraphReadMode.class,
"GraphReadMode");
}
```
##########
hugegraph-loader/assembly/travis/install-hugegraph-from-source.sh:
##########
@@ -41,7 +41,10 @@ mkdir ${HTTPS_SERVER_DIR}
cp -r apache-hugegraph-*/. ${HTTPS_SERVER_DIR}
cd "$(find apache-hugegraph-* | head -1)"
# start HugeGraphServer with http protocol
-bin/init-store.sh || exit 1
+sed -i
's|gremlin.graph=org.apache.hugegraph.HugeFactory|gremlin.graph=org.apache.hugegraph.auth.HugeFactoryAuthProxy|'
conf/graphs/hugegraph.properties
+sed -i
's|#auth.authenticator=.*|auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator|'
conf/rest-server.properties
+sed -i 's|#auth.admin_pa=.*|auth.admin_pa=pa|' conf/rest-server.properties
Review Comment:
⚠️ **测试代码中硬编码认证信息**
在测试脚本中直接硬编码 `admin/pa` 凭证存在以下问题:
1. 降低了测试的灵活性
2. 如果服务器配置改变,测试会失败
3. 不符合配置外部化最佳实践
建议:
- 使用环境变量 `HUGEGRAPH_ADMIN_PASSWORD`
- 或从配置文件读取
- 提供默认值作为后备
```suggestion
ADMIN_PASSWORD=${HUGEGRAPH_ADMIN_PASSWORD:-pa}
sed -i "s|#auth.admin_pa=.*|auth.admin_pa=$ADMIN_PASSWORD|"
conf/rest-server.properties
echo -e "$ADMIN_PASSWORD" | bin/init-store.sh || exit 1
```
--
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]
