You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "sadwitdastreetz (via GitHub)" <gi...@apache.org> on 2025/10/29 08:58:24 UTC
[PR] refactor(loader): adjust LoadContext to 1.7.0 version [incubator-hugegraph-toolchain]
sadwitdastreetz opened a new pull request, #687:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/687
<!--
Thank you very much for contributing to Apache HugeGraph, we are happy that you want to help us improve it!
Here are some tips for you:
1. If this is your first time, please read the [contributing guidelines](https://github.com/apache/hugegraph/blob/master/CONTRIBUTING.md)
2. If a PR fix/close an issue, type the message "close xxx" (xxx is the link of related
issue) in the content, GitHub will auto link it (Required)
3. Name the PR title in "Google Commit Format", start with "feat | fix | perf | refactor | doc | chore",
such like: "feat(core): support the PageRank algorithm" or "fix: wrong break in the compute loop" (module is optional)
skip it if you are unsure about which is the best component.
4. One PR address one issue, better not to mix up multiple issues.
5. Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]` (or click it directly after
published)
-->
## Purpose of the PR
- close #xxx <!-- or use "fix #xxx", "xxx" is the ID-link of related issue, e.g: close #1024 -->
<!--
Please explain more context in this section, clarify why the changes are needed.
e.g:
- If you propose a new API, clarify the use case for a new API.
- If you fix a bug, you can clarify why it is a bug, and should be associated with an issue.
-->
## Main Changes
<!-- Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue. These change logs are helpful for better ant faster reviews.)
For example:
- If you introduce a new feature, please show detailed design here or add the link of design documentation.
- If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
- If there is a discussion in the mailing list, please add the link. -->
## Verifying these changes
<!-- Please pick the proper options below -->
- [ ] Trivial rework / code cleanup without any test coverage. (No Need)
- [ ] Already covered by existing tests, such as *(please modify tests here)*.
- [ ] Need tests and can be verified as follows:
- xxx
## Does this PR potentially affect the following parts?
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] Nope
- [ ] Dependencies (add/update license info) <!-- Don't forget to add/update the info in "LICENSE" & "NOTICE" files (both in root & dist module) -->
- [ ] Modify configurations
- [ ] The public API
- [ ] Other affects (typed here)
## Documentation Status
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `Doc - TODO` <!-- Your PR changes impact docs and you will update later -->
- [ ] `Doc - Done` <!-- Related docs have been already added or updated -->
- [ ] `Doc - No Need` <!-- Your PR changes don't impact/need docs -->
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(loader): adjust LoadContext to 1.7.0 version [incubator-hugegraph-toolchain]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #687:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/687#issuecomment-3468067557
## [Codecov](https://app.codecov.io/gh/apache/incubator-hugegraph-toolchain/pull/687?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `42.10526%` with `11 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 51.83%. Comparing base ([`b066b80`](https://app.codecov.io/gh/apache/incubator-hugegraph-toolchain/commit/b066b805630ca591809cfee2da2aae1395fc6b42?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7bdd211`](https://app.codecov.io/gh/apache/incubator-hugegraph-toolchain/commit/7bdd211d7825de2a5a17a11dc40fe28142d12f8f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 58 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/incubator-hugegraph-toolchain/pull/687?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ava/org/apache/hugegraph/api/graphs/GraphsAPI.java](https://app.codecov.io/gh/apache/incubator-hugegraph-toolchain/pull/687?src=pr&el=tree&filepath=hugegraph-client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Fapi%2Fgraphs%2FGraphsAPI.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVnZWdyYXBoL2FwaS9ncmFwaHMvR3JhcGhzQVBJLmphdmE=) | 42.10% | [10 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph-toolchain/pull/687?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (b066b80) and HEAD (7bdd211). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (b066b80) | HEAD (7bdd211) |
>|------|------|------|
>||2|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #687 +/- ##
=============================================
- Coverage 62.49% 51.83% -10.67%
+ Complexity 1903 1089 -814
=============================================
Files 262 224 -38
Lines 9541 6704 -2837
Branches 886 411 -475
=============================================
- Hits 5963 3475 -2488
+ Misses 3190 3015 -175
+ Partials 388 214 -174
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-hugegraph-toolchain/pull/687?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(loader): adjust LoadContext to 1.7.0 version [incubator-hugegraph-toolchain]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #687:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/687#discussion_r2480387990
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/LoadTest.java:
##########
@@ -146,12 +146,27 @@ public static void assertDateEquals(String expectDate, TimeZone expectZone, Stri
Assert.assertEquals(expectTimeStamp, actualTimeStamp);
}
+ /**
+ * Entry point for running the HugeGraphLoader with authentication parameters.
+ * This method appends authentication arguments (username and password) to the
+ * provided command-line arguments and then invokes {@link HugeGraphLoader#main(String[])}
+ * to start the data loading process.
+ * Specifically, it appends:
+ * --username admin
+ * --password pa
+ * to the end of the original argument list before delegating to HugeGraphLoader.
Review Comment:
⚠️ **代码质量 - LoadTest.authmain() 中的密码硬编码与注释不一致**
在 LoadTest.java 中,`authmain` 方法的 JavaDoc 注释说明密码是 `admin`,但实际代码中密码是 `pa`:
```java
// 注释中写的是:
// --password admin
// 但代码实际是:
list.add("pa");
```
**问题:**
- 注释与代码不一致,会误导开发者
- 虽然这次 PR 修正了代码(从 "admin" 改为 "pa"),但忘记更新注释
**建议:**
更新 JavaDoc 注释中的密码说明:
```suggestion
* --password pa
```
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/LoadTest.java:
##########
@@ -146,12 +146,27 @@ public static void assertDateEquals(String expectDate, TimeZone expectZone, Stri
Assert.assertEquals(expectTimeStamp, actualTimeStamp);
}
+ /**
+ * Entry point for running the HugeGraphLoader with authentication parameters.
+ * This method appends authentication arguments (username and password) to the
+ * provided command-line arguments and then invokes {@link HugeGraphLoader#main(String[])}
+ * to start the data loading process.
+ * Specifically, it appends:
+ * --username admin
+ * --password pa
+ * to the end of the original argument list before delegating to HugeGraphLoader.
+ * @param args the original command-line arguments passed to the program.
+ * These arguments are extended with authentication information
+ * before being passed to {@code HugeGraphLoader.main()}.
+ *
+ * @see HugeGraphLoader#main(String[])
+ */
public static void authmain(String[] args) {
ArrayList list = new ArrayList(Arrays.asList(args));
Review Comment:
🧹 **代码风格 - 可以简化泛型声明**
这行代码可以使用菱形操作符简化:
```suggestion
ArrayList list = new ArrayList<>(Arrays.asList(args));
```
或者更好的做法是添加类型参数:
```java
ArrayList<String> list = new ArrayList<>(Arrays.asList(args));
```
这样可以:
1. 避免原始类型警告
2. 提供更好的类型安全性
3. 代码更清晰易读
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(loader): adjust LoadContext to 1.7.0 version [incubator-hugegraph-toolchain]
Posted by "sadwitdastreetz (via GitHub)" <gi...@apache.org>.
sadwitdastreetz commented on code in PR #687:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/687#discussion_r2480183690
##########
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:
the username and password are already written in `.properties`, the sed command is only to let them show out
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(loader): adjust LoadContext to 1.7.0 version [incubator-hugegraph-toolchain]
Posted by "Pengzna (via GitHub)" <gi...@apache.org>.
Pengzna merged PR #687:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/687
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(loader): adjust LoadContext to 1.7.0 version [incubator-hugegraph-toolchain]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(loader): adjust LoadContext to 1.7.0 version [incubator-hugegraph-toolchain]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #687:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/687#discussion_r2480399286
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/LoadTest.java:
##########
@@ -146,12 +146,27 @@ public static void assertDateEquals(String expectDate, TimeZone expectZone, Stri
Assert.assertEquals(expectTimeStamp, actualTimeStamp);
}
+ /**
+ * Entry point for running the HugeGraphLoader with authentication parameters.
+ * This method appends authentication arguments (username and password) to the
+ * provided command-line arguments and then invokes {@link HugeGraphLoader#main(String[])}
+ * to start the data loading process.
+ * Specifically, it appends:
+ * --username admin
+ * --password pa
+ * to the end of the original argument list before delegating to HugeGraphLoader.
+ * @param args the original command-line arguments passed to the program.
+ * These arguments are extended with authentication information
+ * before being passed to {@code HugeGraphLoader.main()}.
+ *
+ * @see HugeGraphLoader#main(String[])
+ */
public static void authmain(String[] args) {
ArrayList list = new ArrayList(Arrays.asList(args));
Review Comment:
The ArrayList should use generic type parameter `ArrayList<String>` instead of raw type to avoid unchecked warnings and provide type safety.
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/LoadTest.java:
##########
@@ -146,12 +146,27 @@ public static void assertDateEquals(String expectDate, TimeZone expectZone, Stri
Assert.assertEquals(expectTimeStamp, actualTimeStamp);
}
+ /**
+ * Entry point for running the HugeGraphLoader with authentication parameters.
+ * This method appends authentication arguments (username and password) to the
+ * provided command-line arguments and then invokes {@link HugeGraphLoader#main(String[])}
+ * to start the data loading process.
+ * Specifically, it appends:
+ * --username admin
+ * --password pa
+ * to the end of the original argument list before delegating to HugeGraphLoader.
Review Comment:
The documentation states '--password pa' but should be updated to clarify this is a simplified test password. Consider documenting why this specific value was chosen or if it's a placeholder that should be changed in production environments.
```suggestion
* to the end of the original argument list before delegating to HugeGraphLoader.
* <p>
* <b>Note:</b> The password "pa" is a simplified test password used only for testing purposes.
* It is a placeholder and <b>must be changed</b> in production environments to a secure value.
* The choice of "pa" is arbitrary and intended to facilitate automated testing.
```
##########
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:
After removing `System.exit(1)`, the method now returns normally after an error. This could cause confusion as the error is printed but execution continues in the calling context. Consider documenting this behavior change or ensuring callers check for errors appropriately.
```suggestion
System.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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(loader): adjust LoadContext to 1.7.0 version [incubator-hugegraph-toolchain]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #687:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/687#discussion_r2480146866
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/FileLoadTest.java:
##########
@@ -102,7 +102,7 @@ public void testAutoCreateSchema() {
"--batch-insert-threads", "2"
};
- HugeGraphLoader.main(args);
+ authmain(args);
Review Comment:
⚠️ **测试方法命名不清晰**
方法名 `authmain()` 含义不明确:
1. `auth` + `main` 还是 `auto` + `main`?
2. 没有说明它是干什么的
3. 与标准的 `main()` 方法容易混淆
建议使用更描述性的名称:
- `loadWithAuth(String[] args)`
- `executeLoaderWithAuthentication(String[] args)`
- 或者添加 Javadoc 解释其用途
另外,这个方法的实现在 diff 中没有显示,需要确认:
1. 它是否正确处理了认证逻辑
2. 是否有适当的错误处理
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/FileLoadTest.java:
##########
@@ -246,15 +248,16 @@ public void testClearSchemaBeforeLoad() {
"josh,32,Beijing",
"peter,35,Shanghai",
"\"li,nary\",26,\"Wu,han\"");
- String[] args1 = new String[]{
+ List<String> argsList1 = new ArrayList<>(Arrays.asList(
"-f", structPath("clear_schema_before_load/struct.json"),
"-g", GRAPH,
"-h", SERVER,
"--batch-insert-threads", "2",
"--test-mode", "true"
- };
+ ));
+ argsList1.addAll(Arrays.asList("--username", "admin", "--password", "pa"));
Review Comment:
⚠️ **不一致的认证参数传递模式**
在测试代码中发现了两种不同的认证参数传递方式:
**方式一(简单):** 直接调用 `authmain(args)`
```java
authmain(args);
```
**方式二(复杂):** 手动添加 username/password 到参数列表
```java
List<String> argsList = new ArrayList<>(Arrays.asList(...));
argsList.addAll(Arrays.asList("--username", "admin", "--password", "pa"));
HugeGraphLoader loader = new HugeGraphLoader(argsList.toArray(new String[0]));
```
**问题:**
1. 为什么有些测试用方式一,有些用方式二?
2. 这会导致测试维护困难
**建议:**
统一使用一种方式,并提供辅助方法:
```java
private String[] withAuth(String... baseArgs) {
List<String> args = new ArrayList<>(Arrays.asList(baseArgs));
args.addAll(Arrays.asList("--username", "admin", "--password", "pa"));
return args.toArray(new String[0]);
}
```
##########
.github/workflows/loader-ci.yml:
##########
@@ -43,13 +43,13 @@ jobs:
fetch-depth: 2
- name: Install JDK 11
- uses: actions/setup-java@v3
+ uses: actions/setup-java@v4
Review Comment:
🧹 **Minor: GitHub Actions 版本更新很好,但缺少变更说明**
将 actions 从 v3 升级到 v4 是好的维护实践,但建议:
1. 在 PR 描述中说明升级原因(安全更新/新特性/弃用)
2. 确认 v4 的破坏性变更不会影响现有工作流
3. 考虑在单独的 PR 中进行依赖升级,便于回滚
参考:
- `actions/setup-java@v4`
- `actions/cache@v4`
- `actions/codecov-action@v4` (注意这个需要 token 配置)
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(loader): adjust LoadContext to 1.7.0 version [incubator-hugegraph-toolchain]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #687:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/687#discussion_r2480147673
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/KafkaLoadTest.java:
##########
@@ -197,7 +201,7 @@ public void testKafkaCsvFormat() {
"--test-mode", "true"
};
- HugeGraphLoader.main(args);
+ authmain(args);
Review Comment:
Inconsistent indentation: this line uses spaces instead of the standard single tab indentation used elsewhere in the file.
```suggestion
authmain(args);
```
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/HDFSLoadTest.java:
##########
@@ -133,16 +142,19 @@ public void testHDFSWithInvalidCoreSitePath() {
"peter,35,Shanghai",
"\"li,nary\",26,\"Wu,han\"");
- String[] args = new String[]{
+ List<String> argsList = new ArrayList<>(Arrays.asList(
"-f", structPath("hdfs_with_invalid_core_site_path/struct.json"),
"-s", configPath("hdfs_with_invalid_core_site_path/schema.groovy"),
"-g", GRAPH,
"-h", SERVER,
"--batch-insert-threads", "2",
"--test-mode", "true"
- };
+ ));
+ argsList.addAll(Arrays.asList("--username", "admin", "--password", "pa"));
+
+
Review Comment:
Extra blank line added unnecessarily. This creates inconsistent spacing compared to similar code blocks at lines 71-73, 100-102, and 126-128.
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/HDFSLoadTest.java:
##########
@@ -109,16 +115,19 @@ public void testHDFSWithCoreSitePathEmpty() {
"peter,35,Shanghai",
"\"li,nary\",26,\"Wu,han\"");
- String[] args = new String[]{
+ List<String> argsList = new ArrayList<>(Arrays.asList(
"-f", structPath("hdfs_with_empty_core_site_path/struct.json"),
"-s", configPath("hdfs_with_empty_core_site_path/schema.groovy"),
"-g", GRAPH,
"-h", SERVER,
"--batch-insert-threads", "2",
"--test-mode", "true"
- };
+ ));
+ argsList.addAll(Arrays.asList("--username", "admin", "--password", "pa"));
+
+
Review Comment:
Extra blank line added unnecessarily. This creates inconsistent spacing compared to similar code blocks at lines 71-73, 100-102, and 153-155.
```suggestion
```
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/HDFSLoadTest.java:
##########
@@ -160,16 +172,19 @@ public void testHDFSWithUnexistCoreSitePath() {
"peter,35,Shanghai",
"\"li,nary\",26,\"Wu,han\"");
- String[] args = new String[]{
+ List<String> argsList = new ArrayList<>(Arrays.asList(
"-f", structPath("hdfs_with_unexist_core_site_path/struct.json"),
"-s", configPath("hdfs_with_unexist_core_site_path/schema.groovy"),
"-g", GRAPH,
"-h", SERVER,
"--batch-insert-threads", "2",
"--test-mode", "true"
- };
+ ));
+ argsList.addAll(Arrays.asList("--username", "admin", "--password", "pa"));
+
+
Review Comment:
Extra blank line added unnecessarily. This creates inconsistent spacing compared to similar code blocks elsewhere in the file.
```suggestion
```
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/FileLoadTest.java:
##########
@@ -2243,16 +2252,20 @@ public void testReloadJsonFailureFiles() throws IOException,
"\"vadas1\", \"date\": \"2013-02-20 13:00:00\"," +
"\"weight\": 1.0}");
- String[] args = new String[]{
+ List<String> argsList = new ArrayList<>(Arrays.asList(
"-f", structPath("reload_json_failure_files/struct.json"),
"-s", configPath("reload_json_failure_files/schema.groovy"),
"-g", GRAPH,
"-h", SERVER,
"--check-vertex", "true",
"--batch-insert-threads", "2",
"--test-mode", "false"
- };
- HugeGraphLoader loader = new HugeGraphLoader(args);
+ ));
+
+ argsList.addAll(Arrays.asList("--username", "admin", "--password", "pa"));
+
+ HugeGraphLoader loader = new HugeGraphLoader(argsList.toArray(new String[0]));
+
Review Comment:
Extra blank line added unnecessarily, creating inconsistent spacing compared to the pattern used at lines 2063-2065.
```suggestion
HugeGraphLoader loader = new HugeGraphLoader(argsList.toArray(new String[0]));
```
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] refactor(loader): adjust LoadContext to 1.7.0 version [incubator-hugegraph-toolchain]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #687:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/687#discussion_r2480387367
##########
.github/workflows/loader-ci.yml:
##########
@@ -27,8 +27,8 @@ jobs:
TRAVIS_DIR: hugegraph-loader/assembly/travis
STATIC_DIR: hugegraph-loader/assembly/static
# TODO: replace it with the (latest - n) commit id (n >= 15)
- # hugegraph commit date: 2024-12-09
- COMMIT_ID: f838897
+ # hugegraph commit date: 2025-10-30
Review Comment:
‼️ **代码错误 - 日期错误**
注释中的日期 `2025-10-30` 是不正确的。这是一个未来的日期(2025年10月30日),但今天是 2025年10月31日。即使按照今天的日期,这个日期也不合理。
应该修正为实际的提交日期。请检查 hugegraph 仓库的 commit `5b3d295` 的真实提交日期。
##########
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);
Review Comment:
‼️ **代码逻辑问题 - 移除 System.exit(1) 可能导致测试行为不一致**
移除 `System.exit(1)` 调用会改变程序的错误处理行为:
**问题:**
- 原代码在加载失败时会立即终止 JVM 进程(退出码 1)
- 新代码只是 return,程序会继续执行
**影响:**
- 在单元测试中,这可能导致测试无法正确检测失败情况
- 在生产环境中可能导致错误被静默忽略
**建议:**
如果这个改动是为了改善测试体验,建议:
1. 考虑通过抛出异常而不是静默返回
2. 或者添加注释说明为什么移除 System.exit(1)
3. 确保所有调用方都能正确处理这种情况
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org