Conversation
- Poll the Raft readiness endpoint before client tests - Accept only HTTP 200 as a ready PD response - Prevent authentication failures from passing startup checks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3261 +/- ##
============================================
- Coverage 41.57% 34.63% -6.95%
+ Complexity 7311 5915 -1396
============================================
Files 793 774 -19
Lines 69106 66913 -2193
Branches 9258 8943 -315
============================================
- Hits 28730 23173 -5557
- Misses 37098 41023 +3925
+ Partials 3278 2717 -561 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The Java 17 move also changes the published hugegraph-common and hugegraph-rpc jars from Java 8 bytecode to Java 17 bytecode, which downstream projects that still target Java 8 and 11 cannot load. The cluster-test changes in this PR are also not exercised by any CI run on this head, because the Cluster Test CI workflow is disabled in the repository. Evidence: Built hugegraph-common at bdce410 with JDK 17 and ran javap on a compiled class (major version 61; master compiles this module with source/target 1.8). gh api actions/workflows shows Cluster Test CI as disabled_manually and no run exists for this head. The three new minicluster unit tests pass locally on JDK 17.
| @@ -132,8 +130,6 @@ | |||
| <plugin> | |||
| <artifactId>maven-compiler-plugin</artifactId> | |||
There was a problem hiding this 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.compiler.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.
| - 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 \ |
There was a problem hiding this 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.
- remove Commons from required checks - require the Java 17 memory Server check - retain analysis and license checks
- avoid claiming TinkerPop 3.8.1 in the foundation - describe shared TinkerPop and Kryo access needs - retain the test and runtime permission boundary
- update both Store dependencies to Fastjson 1.2.84 - align the release license and dependency inventory - retain the Fastjson 1.x dependency contract
- override the legacy client transitive dependency - retain the existing client and production versions - remove obsolete release inventory entries
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The new FixGraphIdControllerTest never runs in CI, so the controller-level validation that this PR adds to /update_graph_id has no test that runs. Separately, the .asf.yaml change drops the Commons build from the required status checks instead of moving it to Java 17. The two open findings from bitflicker64 at this head (Java 17 bytecode in the published hugegraph-common and hugegraph-rpc jars, and the disabled Cluster Test CI workflow) still apply and are not repeated here. Evidence: store job log for run 37172218875 at 7e6da14 (every surefire:test (default-test) @ hg-store-node reports Tests run: 0 under the auto-detected JUnitPlatformProvider), hugegraph-store/hg-store-test/pom.xml store-core-test includes, and git diff 176fb56d..7e6da142 -- .asf.yaml.
| - check-license | ||
| - build-server (memory, 11) | ||
| - build-commons (11) | ||
| - build-server (memory, 17) |
There was a problem hiding this comment.
Minor: This removes build-commons (11) from the required checks but does not add build-commons (17), so Commons test failures no longer block merges to master.
commons-ci.yml still runs on every pull_request with no path filter, and at this head it reports the check as build-commons (17). Only the server check was renamed to 17. The Commons unit and RPC suites were a merge gate before this PR and are no longer one after it. That matters more in this series, because this PR also changes the Commons compiler target and dependency versions (commons-configuration2 2.10.1, commons-lang3 3.18.0).
Requested change: add - build-commons (17) under contexts, or say in the PR description why Commons is being dropped as a merge gate.
Move existing controller cases into the Store test module. Select the controller class in the core test profile. Retain the existing validation assertions.
Use the existing Node JUnit Platform provider. Keep controller tests alongside the Node implementation. Remove the cross-module core test include.
3 participants
Purpose of the PR
Part 1 of a three-PR upgrade series targeting
apache/hugegraph:master. Establish a Java 17 build and runtime baseline while retaining TinkerPop 3.5.1.Merge order: apache#3261 (Java 17 foundation) → apache#3262 (query semantics) → apache#3263 (TinkerPop upgrade). All three PRs target
masterdirectly. This PR is the prerequisite for the query-semantics and TinkerPop-upgrade PRs.Source: hugegraph/hugegraph#231, submitted directly from
hugegraph:task/tp381-1-java17-foundation. The local validation statements below are carried over from that source PR; CI on this ASF PR head remains the merge gate.Main Changes
Verifying these changes
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODO: coordinate the merge of apache/hugegraph-doc#508 with this upgrade series.Doc - DoneDoc - No NeedRepository documentation is included in
docs/BUILDING.md, the README and PD/Store deployment guides. License declarations and the dependency inventory are updated for the Groovy compatibility pin.