bitflicker64 commented on code in PR #3359:
URL: https://github.com/apache/hugegraph/pull/3359#discussion_r4317533389
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/TaskAPI.java:
##########
@@ -57,8 +59,11 @@ public String patrolStores() {
@ResponseBody
public String patrolPartitions() {
try {
- List<Metapb.Partition> partitions =
pdRestService.patrolPartitions();
- return toJSON(partitions, "partitions");
+ PartitionPatrolResult result =
pdRestService.patrolPartitionsWithResult();
+ String partitions = toJSON(result.getPartitions(), "partitions");
Review Comment:
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.
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/core/PartitionPatrolResultTest.java:
##########
@@ -0,0 +1,178 @@
+/*
+ * 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.
+ */
+
+package org.apache.hugegraph.pd.core;
+
+import java.util.List;
+import java.util.Map;
+
+import org.apache.hugegraph.pd.KvService;
+import org.apache.hugegraph.pd.PartitionService;
+import org.apache.hugegraph.pd.StoreNodeService;
+import org.apache.hugegraph.pd.TaskScheduleService;
+import org.apache.hugegraph.pd.TaskScheduleService.PartitionPatrolResult;
+import org.apache.hugegraph.pd.config.PDConfig;
+import org.apache.hugegraph.pd.grpc.Metapb;
+import org.apache.hugegraph.pd.grpc.Pdpb;
+import org.apache.hugegraph.pd.rest.TaskAPI;
Review Comment:
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 loca
l 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.
--
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]
