bitflicker64 commented on code in PR #3261:
URL: https://github.com/apache/hugegraph/pull/3261#discussion_r4175217760
##########
hugegraph-commons/pom.xml:
##########
@@ -132,8 +130,6 @@
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
Review Comment:
Important: removing `<source>${compiler.source}</source>` /
`<target>${compiler.target}</target>` (1.8) here means hugegraph-common and
hugegraph-rpc now inherit `maven.compiler.release=17` from the root pom. These
are published library artifacts, not only server internals. I built
hugegraph-common at this head with JDK 17 and `javap -v` on a compiled class
reports `major version: 61` (Java 17). On master the module compiles to Java 8
bytecode. apache/hugegraph-toolchain consumes `hugegraph-common` through
`${hugegraph.version}` and still compiles with `compiler.source/target 1.8` and
runs client CI on Java 11. hugegraph-computer also depends on it with target 11
and CI on Java 8 and 11. Once a release from this line ships, those projects
get `UnsupportedClassVersionError` when they bump the dependency. The PR
description lists the server/PD/Store runtime JDK as affected but not the
commons libraries. Please either keep `hugegraph-commons` on a lower release,
for example `<maven.c
ompiler.release>8</maven.compiler.release>` (or 11) in
`hugegraph-commons/pom.xml`, or state the commons baseline change explicitly in
the PR and line it up with toolchain and computer before merge.
##########
hugegraph-server/hugegraph-test/conf/jvm-test-module.options:
##########
@@ -0,0 +1,29 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Test-only access required by TinkerPop 3.8.1 and its Kryo dependencies on
Review Comment:
Minor: this comment says the options are required by TinkerPop 3.8.1, but
this PR keeps TinkerPop 3.5.1 (`hugegraph-struct/pom.xml` still sets
`tinkerpop.version` to 3.5.1 and the root pom comment says the Groovy pin is
kept until the TinkerPop upgrade). Please reword it to describe the current
3.5.1 and Kryo needs, or move the 3.8.1 wording to part 3 of the series, so the
file does not describe a dependency that is not on the classpath yet.
##########
.github/workflows/cluster-test-ci.yml:
##########
@@ -63,8 +63,27 @@ jobs:
- name: Run simple cluster test
run: |
- mvn test -pl hugegraph-cluster-test/hugegraph-clustertest-test -am
-P simple-cluster-test
+ timeout 45m mvn test -pl
hugegraph-cluster-test/hugegraph-clustertest-test \
Review Comment:
Important: this PR changes the cluster-test lifecycle (`AbstractEnv` start
timeouts, `ServerNodeWrapper` port readiness, Gremlin port rewriting,
`pd.peers` in the template) and adds `EnvUtilTest`, `GremlinServerConfigTest`
and `AbstractNodeWrapperTest`, but none of it ran on this head. `gh api
repos/apache/hugegraph/actions/workflows` reports `Cluster Test CI` as
`disabled_manually`, and its last run was in October 2025. No other active
workflow builds `hugegraph-cluster-test` with tests. The PR body says CI on
this head is the merge gate, so the lane that covers the "cluster test
foundation" part of the title is missing from that gate. I ran `mvn test -pl
hugegraph-cluster-test/hugegraph-clustertest-minicluster` locally on JDK 17 and
the three new unit tests pass. The simple and multi cluster suites, which
depend on the new readiness logic, are still unverified. Please get Cluster
Test CI re-enabled and run against this head, or link a passing run of both
cluster profiles on this
exact SHA, before merge.
--
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]
