imbajin commented on code in PR #2921:
URL:
https://github.com/apache/incubator-hugegraph/pull/2921#discussion_r2659463098
##########
hugegraph-cluster-test/hugegraph-clustertest-minicluster/src/main/java/org/apache/hugegraph/ct/node/ServerNodeWrapper.java:
##########
@@ -34,15 +34,20 @@
import static
org.apache.hugegraph.ct.base.ClusterConstant.SERVER_TEMPLATE_PATH;
import static org.apache.hugegraph.ct.base.ClusterConstant.isJava11OrHigher;
+import java.io.BufferedReader;
import java.io.File;
import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.nio.charset.StandardCharsets;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
public class ServerNodeWrapper extends AbstractNodeWrapper {
Review Comment:
**‼️ Critical: Static list not thread-safe**
The static `hgJars` list is modified in instance method `loadHgJars()` which
gets called in the constructor. If multiple ServerNodeWrapper instances are
created concurrently, this can lead to race conditions.
**Recommendation:**
```java
// Make it thread-safe and ensure single initialization
private static final List<String> hgJars = loadHgJarsOnce();
private static List<String> loadHgJarsOnce() {
List<String> jars = new ArrayList<>();
try (InputStream is =
ServerNodeWrapper.class.getResourceAsStream("/jar.txt");
BufferedReader reader = new BufferedReader(new
InputStreamReader(is, StandardCharsets.UTF_8))) {
String line;
while ((line != reader.readLine()) != null) {
jars.add(line);
}
} catch (IOException e) {
throw new RuntimeException("Failed to load jar list", e);
}
return Collections.unmodifiableList(jars);
}
```
Remove the `loadHgJars()` call from the constructor.
##########
hugegraph-cluster-test/hugegraph-clustertest-minicluster/src/main/java/org/apache/hugegraph/ct/node/ServerNodeWrapper.java:
##########
@@ -67,6 +73,36 @@ private static void addJarsToClasspath(File directory,
List<String> classpath) {
}
}
+ private static void addCpJarsToClasspath(File directory, List<String>
classpath) {
Review Comment:
**⚠️ Code quality: Inconsistent naming pattern**
The method name `addCpJarsToClasspath` doesn't clearly communicate what 'Cp'
means. Consider a more descriptive name.
**Suggestion:**
```java
private static void addOrderedHugeGraphJarsToClasspath(File directory,
List<String> classpath)
```
This makes it clear that this method adds HugeGraph jars in a specific order.
##########
hugegraph-cluster-test/hugegraph-clustertest-minicluster/src/main/java/org/apache/hugegraph/ct/node/ServerNodeWrapper.java:
##########
@@ -67,6 +73,36 @@ private static void addJarsToClasspath(File directory,
List<String> classpath) {
}
}
+ private static void addCpJarsToClasspath(File directory, List<String>
classpath) {
+ // Add jar starts with hugegraph in proper order
+ String path = directory.getAbsolutePath();
+ for (String jar : hgJars) {
+ classpath.add(path + File.separator + jar);
+ }
+ if (directory.exists() && directory.isDirectory()) {
+ File[] files =
+ directory.listFiles((dir, name) -> name.endsWith(".jar")
&& !name.contains(
+ "hugegraph"));
+ if (files != null) {
+ for (File file : files) {
+ classpath.add(file.getAbsolutePath());
+ }
+ }
+ }
+ }
+
Review Comment:
**🧹 Minor: Resource file missing from PR**
The code references `/jar.txt` resource file but it's not included in this
PR. Ensure this file exists in `src/main/resources/jar.txt` and is
generated/populated correctly during the build.
Verify that the maven-resources-plugin configuration in pom.xml properly
filters this file.
##########
hugegraph-cluster-test/hugegraph-clustertest-dist/src/assembly/static/conf/hugegraph.properties.template:
##########
@@ -16,8 +16,7 @@
#
# gremlin entrance to create graph
-# auth config: org.apache.hugegraph.auth.HugeFactoryAuthProxy
-gremlin.graph=org.apache.hugegraph.HugeFactory
+gremlin.graph=org.apache.hugegraph.auth.HugeFactoryAuthProxy
Review Comment:
**⚠️ Potential issue: Changing from HugeFactory to HugeFactoryAuthProxy**
This changes the default behavior to require authentication. This is a
**breaking change** that should be:
1. Clearly documented in the PR description
2. Mentioned in migration/upgrade guide
3. Considered for backwards compatibility
**Questions:**
- Is this intentional for all cluster test scenarios?
- Are all existing tests updated to handle authentication?
- Should this be configurable rather than hardcoded?
--
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]
