You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@helix.apache.org by hu...@apache.org on 2019/06/24 23:49:29 UTC
[helix] 07/15: Change output behavior for non-exist instances
This is an automated email from the ASF dual-hosted git repository.
hulee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git
commit 0e6cccbaf2e7a0c0fd4c1e3da6d5df349f3dedf6
Author: Junkai Xue <jx...@linkedin.com>
AuthorDate: Thu May 30 16:47:16 2019 -0700
Change output behavior for non-exist instances
Current behavior of non-existing instance will be not showing in output. So user is hard to differentiate whether instance does not exist or not belongs to same zone.
Add the logic to check instances exists in instance list or not.
RB=1684700
BUG=HELIX-1911
G=helix-reviewers
A=hulee
Signed-off-by: Hunter Lee <hu...@linkedin.com>
---
.../rest/server/json/cluster/ClusterTopology.java | 10 +++++-
.../server/resources/helix/InstancesAccessor.java | 39 ++++++++++++++++------
.../rest/server/service/ClusterServiceImpl.java | 5 ++-
.../helix/rest/server/TestClusterAccessor.java | 5 ++-
.../helix/rest/server/TestInstancesAccessor.java | 7 ++--
.../server/json/cluster/TestClusterTopology.java | 6 ++--
6 files changed, 54 insertions(+), 18 deletions(-)
diff --git a/helix-rest/src/main/java/org/apache/helix/rest/server/json/cluster/ClusterTopology.java b/helix-rest/src/main/java/org/apache/helix/rest/server/json/cluster/ClusterTopology.java
index 8325822..701e4bd 100644
--- a/helix-rest/src/main/java/org/apache/helix/rest/server/json/cluster/ClusterTopology.java
+++ b/helix-rest/src/main/java/org/apache/helix/rest/server/json/cluster/ClusterTopology.java
@@ -23,10 +23,14 @@ public class ClusterTopology {
private final String clusterId;
@JsonProperty("zones")
private List<Zone> zones;
+ @JsonProperty("allInstances")
+ private Set<String> allInstances;
- public ClusterTopology(String clusterId, List<Zone> zones) {
+
+ public ClusterTopology(String clusterId, List<Zone> zones, Set<String> allInstances) {
this.clusterId = clusterId;
this.zones = zones;
+ this.allInstances = allInstances;
}
public String getClusterId() {
@@ -37,6 +41,10 @@ public class ClusterTopology {
return zones;
}
+ public Set<String> getAllInstances() {
+ return allInstances;
+ }
+
public static final class Zone {
@JsonProperty("id")
private final String id;
diff --git a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java
index 99449be..36e7249 100644
--- a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java
+++ b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java
@@ -24,6 +24,7 @@ import org.apache.helix.manager.zk.ZKHelixDataAccessor;
import org.apache.helix.model.ClusterConfig;
import org.apache.helix.model.InstanceConfig;
import org.apache.helix.rest.common.HelixDataAccessorWrapper;
+import org.apache.helix.rest.server.json.cluster.ClusterTopology;
import org.apache.helix.rest.server.json.instance.StoppableCheck;
import org.apache.helix.rest.server.resources.exceptions.HelixHealthException;
import org.apache.helix.rest.server.service.ClusterService;
@@ -40,7 +41,10 @@ import org.slf4j.LoggerFactory;
@Path("/clusters/{clusterId}/instances")
public class InstancesAccessor extends AbstractHelixResource {
private final static Logger _logger = LoggerFactory.getLogger(InstancesAccessor.class);
-
+ // This type does not belongs to real HealthCheck failed reason. Also if we add this type
+ // to HealthCheck enum, it could introduce more unnecessary check step since the InstanceServiceImpl
+ // loops all the types to do corresponding checks.
+ private final static String INSTANCE_NOT_EXIST = "Helix:INSTANCE_NOT_EXIST";
public enum InstancesProperties {
instances,
online,
@@ -57,7 +61,6 @@ public class InstancesAccessor extends AbstractHelixResource {
zone_based
}
-
@GET
public Response getAllInstances(@PathParam("clusterId") String clusterId) {
HelixDataAccessor accessor = getDataAccssor(clusterId);
@@ -174,9 +177,13 @@ public class InstancesAccessor extends AbstractHelixResource {
InstancesAccessor.InstancesProperties.instance_not_stoppable_with_reasons.name());
InstanceService instanceService =
new InstanceServiceImpl(new HelixDataAccessorWrapper((ZKHelixDataAccessor) getDataAccssor(clusterId)), getConfigAccessor());
+ ClusterService clusterService =
+ new ClusterServiceImpl(getDataAccssor(clusterId), getConfigAccessor());
+ ClusterTopology clusterTopology = clusterService.getClusterTopology(clusterId);
switch (selectionBase) {
case zone_based:
- List<String> zoneBasedInstance = getZoneBasedInstances(clusterId, instances, orderOfZone);
+ List<String> zoneBasedInstance =
+ getZoneBasedInstances(instances, orderOfZone, clusterTopology.toZoneMapping());
for (String instance : zoneBasedInstance) {
StoppableCheck stoppableCheckResult =
instanceService.getInstanceStoppableCheck(clusterId, instance, customizedInput);
@@ -189,6 +196,22 @@ public class InstancesAccessor extends AbstractHelixResource {
stoppableInstances.add(instance);
}
}
+
+ // Adding following logic to check whether instances exist or not. An instance exist could be
+ // checking following scenario:
+ // 1. Instance got dropped. (InstanceConfig is gone.)
+ // 2. Instance name has typo.
+
+ // If we dont add this check, the instance, which does not exist, will be disappeared from
+ // result since Helix skips instances for instances not in the selected zone. User may get
+ // confused with the output.
+ Set<String> nonSelectedInstances = new HashSet<>(instances);
+ nonSelectedInstances.removeAll(clusterTopology.getAllInstances());
+ for (String nonSelectedInstance : nonSelectedInstances) {
+ ArrayNode failedReasonsNode = failedStoppableInstances.putArray(nonSelectedInstance);
+ failedReasonsNode.add(JsonNodeFactory.instance.textNode(INSTANCE_NOT_EXIST));
+ }
+
break;
case instance_based:
default:
@@ -214,17 +237,13 @@ public class InstancesAccessor extends AbstractHelixResource {
* The order of zones can directly come from user input. If user did not specify it, Helix will order
* zones with alphabetical order.
*
- * @param clusterId
* @param instances
* @param orderedZones
* @return
*/
- private List<String> getZoneBasedInstances(String clusterId, List<String> instances,
- List<String> orderedZones) {
- ClusterService clusterService =
- new ClusterServiceImpl(getDataAccssor(clusterId), getConfigAccessor());
- Map<String, Set<String>> zoneMapping =
- clusterService.getClusterTopology(clusterId).toZoneMapping();
+ private List<String> getZoneBasedInstances(List<String> instances, List<String> orderedZones,
+ Map<String, Set<String>> zoneMapping) {
+
if (orderedZones == null) {
orderedZones = new ArrayList<>(zoneMapping.keySet());
}
diff --git a/helix-rest/src/main/java/org/apache/helix/rest/server/service/ClusterServiceImpl.java b/helix-rest/src/main/java/org/apache/helix/rest/server/service/ClusterServiceImpl.java
index 5fb9c61..9f20a02 100644
--- a/helix-rest/src/main/java/org/apache/helix/rest/server/service/ClusterServiceImpl.java
+++ b/helix-rest/src/main/java/org/apache/helix/rest/server/service/ClusterServiceImpl.java
@@ -5,6 +5,7 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.stream.Collectors;
import org.apache.helix.AccessOption;
import org.apache.helix.ConfigAccessor;
import org.apache.helix.HelixDataAccessor;
@@ -56,7 +57,9 @@ public class ClusterServiceImpl implements ClusterService {
zones.add(zone);
}
- return new ClusterTopology(cluster, zones);
+ // Get all the instances names
+ return new ClusterTopology(cluster, zones,
+ instanceConfigs.stream().map(InstanceConfig::getInstanceName).collect(Collectors.toSet()));
}
@Override
diff --git a/helix-rest/src/test/java/org/apache/helix/rest/server/TestClusterAccessor.java b/helix-rest/src/test/java/org/apache/helix/rest/server/TestClusterAccessor.java
index d07c55f..ab43ae0 100644
--- a/helix-rest/src/test/java/org/apache/helix/rest/server/TestClusterAccessor.java
+++ b/helix-rest/src/test/java/org/apache/helix/rest/server/TestClusterAccessor.java
@@ -113,7 +113,10 @@ public class TestClusterAccessor extends AbstractTestClass {
String response = new JerseyUriRequestBuilder("clusters/{}/topology").format(cluster).get(this);
Assert.assertEquals(response,
- "{\"id\":\"TestCluster_1\",\"zones\":[{\"id\":\"123\",\"instances\":[{\"id\":\"TestCluster_1localhost_12920\"}]}]}");
+ "{\"id\":\"TestCluster_1\",\"zones\":[{\"id\":\"123\",\"instances\":[{\"id\":\"TestCluster_1localhost_12920\"}]}],"
+ + "\"allInstances\":[\"TestCluster_1localhost_12918\",\"TestCluster_1localhost_12919\",\"TestCluster_1localhost_12924\","
+ + "\"TestCluster_1localhost_12925\",\"TestCluster_1localhost_12926\",\"TestCluster_1localhost_12927\",\"TestCluster_1localhost_12920\","
+ + "\"TestCluster_1localhost_12921\",\"TestCluster_1localhost_12922\",\"TestCluster_1localhost_12923\"]}");
}
@Test(dependsOnMethods = "testGetClusterTopology")
diff --git a/helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java b/helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java
index 0e86584..78bce96 100644
--- a/helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java
+++ b/helix-rest/src/test/java/org/apache/helix/rest/server/TestInstancesAccessor.java
@@ -30,11 +30,11 @@ public class TestInstancesAccessor extends AbstractTestClass {
System.out.println("Start test :" + TestHelper.getTestMethodName());
// Select instances with zone based
String content =
- String.format("{\"%s\":\"%s\",\"%s\":[\"%s\",\"%s\",\"%s\",\"%s\",\"%s\",\"%s\"]}",
+ String.format("{\"%s\":\"%s\",\"%s\":[\"%s\",\"%s\",\"%s\",\"%s\",\"%s\",\"%s\", \"%s\"]}",
InstancesAccessor.InstancesProperties.selection_base.name(),
InstancesAccessor.InstanceHealthSelectionBase.zone_based.name(),
InstancesAccessor.InstancesProperties.instances.name(), "instance0", "instance1",
- "instance2", "instance3", "instance4", "instance5");
+ "instance2", "instance3", "instance4", "instance5", "invalidInstance");
Response response = new JerseyUriRequestBuilder("clusters/{}/instances?command=stoppable")
.format(STOPPABLE_CLUSTER)
.post(this, Entity.entity(content, MediaType.APPLICATION_JSON_TYPE));
@@ -45,7 +45,8 @@ public class TestInstancesAccessor extends AbstractTestClass {
+ " \"instance1\" : [ \"Helix:EMPTY_RESOURCE_ASSIGNMENT\", \"Helix:INSTANCE_NOT_ENABLED\", \"Helix:INSTANCE_NOT_STABLE\" ],\n"
+ " \"instance2\" : [ \"Helix:MIN_ACTIVE_REPLICA_CHECK_FAILED\" ],\n"
+ " \"instance3\" : [ \"Helix:HAS_DISABLED_PARTITION\", \"Helix:MIN_ACTIVE_REPLICA_CHECK_FAILED\" ],\n"
- + " \"instance4\" : [ \"Helix:EMPTY_RESOURCE_ASSIGNMENT\", \"Helix:INSTANCE_NOT_ALIVE\", \"Helix:INSTANCE_NOT_STABLE\" ]\n"
+ + " \"instance4\" : [ \"Helix:EMPTY_RESOURCE_ASSIGNMENT\", \"Helix:INSTANCE_NOT_ALIVE\", \"Helix:INSTANCE_NOT_STABLE\" ],\n"
+ + " \"invalidInstance\" : [ \"Helix:INSTANCE_NOT_EXIST\" ]\n"
+ " }\n" + "}\n");
}
diff --git a/helix-rest/src/test/java/org/apache/helix/rest/server/json/cluster/TestClusterTopology.java b/helix-rest/src/test/java/org/apache/helix/rest/server/json/cluster/TestClusterTopology.java
index eadaa58..cbb0081 100644
--- a/helix-rest/src/test/java/org/apache/helix/rest/server/json/cluster/TestClusterTopology.java
+++ b/helix-rest/src/test/java/org/apache/helix/rest/server/json/cluster/TestClusterTopology.java
@@ -1,6 +1,7 @@
package org.apache.helix.rest.server.json.cluster;
import java.io.IOException;
+import java.util.Collections;
import java.util.List;
import org.testng.Assert;
@@ -18,11 +19,12 @@ public class TestClusterTopology {
List<ClusterTopology.Zone> zones = ImmutableList.of(new ClusterTopology.Zone("zone", instances));
- ClusterTopology clusterTopology = new ClusterTopology("cluster0", zones);
+ ClusterTopology clusterTopology =
+ new ClusterTopology("cluster0", zones, Collections.emptySet());
ObjectMapper mapper = new ObjectMapper();
String result = mapper.writeValueAsString(clusterTopology);
Assert.assertEquals(result,
- "{\"id\":\"cluster0\",\"zones\":[{\"id\":\"zone\",\"instances\":[{\"id\":\"instance\"}]}]}");
+ "{\"id\":\"cluster0\",\"zones\":[{\"id\":\"zone\",\"instances\":[{\"id\":\"instance\"}]}],\"allInstances\":[]}");
}
}