bitflicker64 commented on PR #3359: URL: https://github.com/apache/hugegraph/pull/3359#issuecomment-6299808750
The earlier review comments below still apply at 5fe9b27; this head does not fix them. - Critical: `PartitionPatrolResultTest` lives in `hg-pd-test/src/main/java`, so the module's main compile has to resolve `TaskAPI` and `PDRestService` from the `hg-pd-service` artifact. In a reactor `package` build that artifact is the Spring Boot repackaged jar, where these classes sit under `BOOT-INF/classes/`, so this import and the next one fail with `cannot find symbol` and the whole build stops at `hg-pd-test`. This is why the `Package` step fails at this head in the `pd_store / pd`, `pd_store / store` and `cluster / cluster-test` jobs of run 39293045735, with exactly these two errors at `PartitionPatrolResultTest.java:[31,36]` and `[32,39]`, so none of the PD tests, including the new ones, ran in CI. I reproduced it locally with `mvn -o package -pl hugegraph-pd/hg-pd-test -am -Dmaven.test.skip=true` (JDK 17): `hg-pd-service` SUCCESS, `hg-pd-test` FAILURE with the same two errors, and `unzip -l` shows `BOOT-INF/classes/org/apache/hugegraph/pd/rest/TaskAPI.class`. The PR's lo cal runs passed because `mvn test` resolves the reactor module to `target/classes` instead of the repackaged jar. No other main source in `hg-pd-test` imports a class from `hg-pd-service`. Please move `testLeadershipChangeAfterPatrolReturnsStructuredRefusal` and `testCompactionRefusalSurvivesLeadershipRegain` into `hg-pd-service/src/test/java` (for example into `TaskAPIResultTest`, which already builds `TaskAPI` with a stubbed `PDRestService`), drop these two imports here, and confirm the root `mvn clean package -Dmaven.test.skip=true` passes. (https://github.com/apache/hugegraph/pull/3359#discussion_r4317533361) - Important: `result` can be null here, and then this line throws a `NullPointerException` that the `catch (PDException e)` does not catch, so the request fails with a 500 and no `status` envelope. `TaskScheduleService.patrolPartitionsWithResult()` returns null whenever its own `isLeader()` check fails. `runLeaderTask` only rejects that when its outer checks also see a follower, so the case where the scheduler saw a follower but both outer checks saw a leader reaches this line. That is the same case this PR already handles for compaction (`PartitionPatrolResultTest.testCompactionRefusalSurvivesLeadershipRegain` stubs the scheduler with its follower return while `isLeader(true)` stays true). I ran the same setup for the other task routes at this head: `patrolPartitions` threw `NullPointerException: Cannot invoke "...PartitionPatrolResult.getPartitions()" because "result" is null`, while `patrolStores` returned `{"status": 0,"stores": [ ]}`, `splitPartitions` `{"status": 0,"partitio ns": [ ]}`, `balanceLeaders` `{}` and `balancePartitions` `{"outcome":"skipped","status":0}`, all success shapes for work that never ran. So the guarantee in the `runLeaderTask` comment holds only for compaction. Please map the scheduler's follower result to `NOT_LEADER` in `PDRestService` the same way `dbCompaction()` now does for `patrolPartitionsWithResult()` and `patrolStores()`, where null means only follower, and add a null check here. `autoSplitPartition()`, `balancePartitionShard()` and `balancePartitionLeader()` also return null or an empty map on other paths, so they need a distinct follower signal from the scheduler (for example throwing `NOT_LEADER` there) rather than a null check. Please also add a regression next to the compaction one. (https://github.com/apache/hugegraph/pull/3359#discussion_r4317533389) -- 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]
