fix(pd): distinguish maintenance task results from follower no-ops

[pallavithegod]

Main Changes (主要改动)

Close #3231.

• Root cause: TaskAPI.balanceLeaders() could leak a maintenance exception, and PDRestService could report follower/skipped work like completed maintenance. Patrol results omitted membership changes made by tombstone reallocation. Compaction discarded the scheduler's refusal, so losing and regaining leadership could still produce compact ok.
• Fix details: Return structured refusals and explicit skipped outcomes, include actual deduplicated patrol changes, and preserve the existing void entry point. Keep compaction success as text/plain and errors as application/json. Propagate a false compaction scheduling result even if leadership has returned by the final check. Results describe scheduling, not completed repair.
• Tests: TaskAPIResultTest pins task outcomes and content types; BalanceLeadersAPITest covers both aliases. PartitionPatrolResultTest.testCompactionRefusalSurvivesLeadershipRegain checks the latest race and preserves successful compaction, alongside patrol deduplication and leadership-change cases.

Verifying these changes (验证改动)

• Added/updated unit tests in TaskAPIResultTest, BalanceLeadersAPITest and PartitionPatrolResultTest
• Verified repository mvn checkstyle:check and mvn apache-rat:check pass locally
• Ran editorconfig:format, clean compilation and all 15 focused task cases on Java 17
• Doc - Done: PD API reference and chart operations; paired website PR docs(dist): explain pd and store recovery signals hugegraph-doc#532.

Current-head upstream CI still requires maintainer approval to run. Inspect cluster state before retrying a leadership-change refusal.

Visual overview

Conceptual compaction example: preserve an explicit scheduler refusal even when PD leadership returns before the final check, rather than reporting compact ok. The broader maintenance API distinguishes refusals and skipped work from accepted scheduling; scheduling does not prove completed repair.

[imbajin]

First independent cross/adversarial review of the reviewed revision: 8/10 overall. The complete patch and scheduler/API paths were independently cross-reviewed; no confirmed source blocker was found.

The before/after leader checks, structured refusal, changed-group reporting, request-versus-completed distinction, skipped-versus-empty results and legacy method signature are coherent. The tests cover follower refusal across routes and the alias, response contents, empty/skipped results and request deduplication. A regression that changes leadership during execution would strengthen coverage; no new blocker has been demonstrated from that gap.

No local test execution was performed for this review. Current CI is action_required, with zero check runs. Wait for actual CI and focused acceptance; this is not a merge-ready claim or approval.

[imbajin]

Blocking: no. Summary: The patrol response omits a tombstone-triggered reallocation, and the compact error response does not declare JSON. Evidence: exact-head static review of TaskScheduleService.java and TaskAPI.java.

[bitflicker64]

Score: 7.5/10. Blocking: no.

Summary: The leader checks before and after each task, the NOT_LEADER refusal with the leader address, the new patrol fields and the skipped versus empty balance result all work as described. I have nothing new to add beyond the two existing threads.

Earlier threads, both still open at 277bfea:

• TaskScheduleService.java:297 (@imbajin), storeTurnoff() reallocation not reported in reallocatedShardGroups: I agree, and I also count it as non-blocking. The reallocation itself is correct; only the report is incomplete. The path only does work when the turnoff that runs as the store becomes Tombstone did not finish, and the group ID still shows up in offlineShardRequests and partitions. Nothing in the repo consumes the field yet, but the docs call it the list of changed groups, so it should be fixed.
• TaskAPI.java:117 (@imbajin), /compact error body sent without a JSON content type: I agree. I count it as Minor because the success body was already plain text.

Score: 10 - 2 (Important) - 0.5 (Minor) = 7.5.

Evidence: ran TaskAPIResultTest, BalanceLeadersAPITest and PartitionPatrolResultTest at 277bfea (10 tests, all pass). Traced StoreNodeService.storeTurnoff() -> reallocShards() and PDRestService.runLeaderTask(). The post-run leader check that can refuse after work has already been sent is covered in the api-reference text (inspect cluster state before retrying), so I am not raising it.

Evidence gaps: no CI has run on this head, and no test covers a leadership change while a task is running.

[pallavithegod]

Thank you for reviewing the task-response changes and pointing out the two non-blocking issues. I'll prepare the follow-up commits after #3231 is assigned to me.

[imbajin]

Independent re-review of the current revision: 8.5/10 overall. No confirmed new blocking defect found; wait for executed CI and focused acceptance.

The two earlier nonblocking points are repaired:

• StoreNodeService / tombstone accounting now feeds changed IDs into patrol's deduplicated result, including the correct-initial-count tombstone path, while retaining the legacy void entrypoint.
• Compact responses explicitly preserve text/plain success and declare application/json errors. The added regression also covers loss of leadership after scheduling.

Independent bounded validation on cloud Java21: 76 assertions passed using exact extracted production patrol/turnoff and leader-wrapper bodies with controlled dependency stubs. This covered initial membership counts 1/2/3, duplicate partition entries, changed-ID/request deduplication, no-op/follower cases, pre/post leader refusal and busy-exception preservation. Replacing the tombstone aggregation with the old implementation fails the correct-count changed-ID assertion, demonstrating that this regression detects the prior omission.

Evidence boundary: these probes do not execute the native JUnit suite, actual allocator/Raft persistence, Spring HTTP stack, JDK17, multi-PD failure recovery, performance or same-RC acceptance. The reported 14 native tests remain author evidence. Exact-head CI is still action_required with zero check runs. Keep the documented distinction between a scheduled request/plan and completed repair, and inspect cluster state before retrying a leadership-change refusal. This review is not an approval or a merge-ready claim.

[imbajin]

Blocking: no. Summary: The new task response contract still has a leadership-transition race that can return success without running a maintenance task. Evidence: Static trace at this head; tests were not run.

[imbajin]

Independent follow-up at 14a91a497: 9/10 overall; no new blocking finding.

The compaction wrapper now preserves the scheduler's explicit refusal. If the outer leadership checks would both pass but scheduling observed a follower in between, the request raises NOT_LEADER instead of returning compact ok. The scheduler's false return is specifically the follower refusal, so this mapping is justified.

Fresh bounded cloud Java21 execution of the exact extracted compaction and leader-wrapper methods passed eight assertions, covering refusal with otherwise stable outer leadership, successful scheduling, pre/post leadership refusal and preservation of a scheduler exception. Restoring the old discarded-result body reproduces false success. This is a dependency-stubbed method probe, not native JUnit, Spring HTTP or distributed leader-term fencing. The new checked-in regression also asserts the structured refusal and unchanged success body; the author's 15 passing focused cases remain author evidence.

The fix closes the reported compaction race; it does not establish transactional maintenance or completed repair, and the existing inspect-before-retry guidance remains appropriate. Exact-head CI is still action_required with zero check runs. Wait for executed CI and focused acceptance; this is a COMMENT, not an approval or merge-ready claim.

---

❕: CI failed ⚠️

[imbajin]

CI follow-up at 14a91a497e4ac3f0de075155bf3440628a981609: the native packaging build now has a confirmed blocker. This qualifies my earlier bounded-method assessment.

PD CI attempt 2 fails during hg-pd-test main-source compilation: the added PartitionPatrolResultTest imports cannot resolve TaskAPI and PDRestService. The command is mvn clean package -U -Dmaven.javadoc.skip=true -Dmaven.test.skip=true -ntp --fail-at-end; skipping tests does not skip compilation of tests placed under src/main/java.

The logs show hg-pd-service replacing its main artifact with the Spring Boot repackaged archive before this downstream compilation. Together with the unchanged no-classifier repackage configuration, this strongly points to an existing packaging limitation exposed by the new service-class imports. The same package command passes in the comparison CI, so this should not be dismissed as a generic baseline outage. This diagnosis is based on source and CI logs; I have not reproduced a proposed fix locally.

Please move the service-facing assertions into hg-pd-service/src/test/java, keep the core-only tests in hg-pd-test, and adjust suite/CI wiring so the moved regressions actually execute. Then rerun the clean package command and focused tests. No new production-behavior regression is demonstrated here, but the packaging failure must be fixed before merge. The separate hstore Netty download HTTP 502 is independent of this compile failure.

[bitflicker64]

Blocking: yes. Summary: The new PartitionPatrolResultTest imports TaskAPI and PDRestService from the repackaged hg-pd-service jar, which breaks the repository package build, and TaskAPI.patrolPartitions() throws a NullPointerException in the leadership case this PR already handles for compaction. The earlier three review threads (tombstone reallocations in reallocatedShardGroups, compact content types, compaction refusal after leadership returns) are fixed at this head. Evidence: read the full diff against merge base be68aa2 plus StoreNodeService.reallocShards()/storeTurnoff(), the TaskScheduleService task methods, API.toJSON, the hg-pd-test and hg-pd-service poms and pd-store-ci.yml. With JDK 17 at 14a91a4, mvn -o test -pl hugegraph-pd/hg-pd-test -am -P pd-core-test passed TaskAPIResultTest (5), BalanceLeadersAPITest (4) and PartitionPatrolResultTest (6); mvn -o package -pl hugegraph-pd/hg-pd-test -am -Dmaven.test.skip=true failed in hg-pd-test with the same cannot find symbol errors as the Package step of CI run 39293045735 (pd_store / pd, pd_store / store, cluster / cluster-test; the pd_store / hstore failure is an unrelated netty dependency resolution error). A temporary probe test, not kept, stubbed the scheduler's follower returns under a leader node and showed the null pointer and the success-shaped bodies described inline.

[bitflicker64]

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 local 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.

[bitflicker64]

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,"partitions": [ ]}, 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.