[GitHub] [skywalking] wu-sheng commented on a change in pull request #5661: Add health checker for cluster management module
wu-sheng commented on a change in pull request #5661:
URL: https://github.com/apache/skywalking/pull/5661#discussion_r507741957
##
File path:
oap-server/server-core/src/test/java/org/apache/skywalking/oap/server/core/cluster/OAPNodeCheckerTest.java
##
@@ -0,0 +1,58 @@
+/*
+ * 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.skywalking.oap.server.core.cluster;
+
+import com.google.common.collect.Sets;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Set;
+
+import static org.hamcrest.CoreMatchers.is;
+
+public class OAPNodeCheckerTest {
+
+@Test
+public void hasUnHealthAddressFalse() {
+Set address = Sets.newHashSet("123.23.4.2");
+boolean flag = OAPNodeChecker.hasUnHealthAddress(address);
+Assert.assertThat(flag, is(false));
Review comment:
There is `Assert.assertFalse()`
##
File path:
oap-server/server-cluster-plugin/cluster-etcd-plugin/src/main/java/org/apache/skywalking/oap/server/cluster/plugin/etcd/EtcdCoordinator.java
##
@@ -51,6 +59,9 @@
private static final Integer KEY_TTL = 45;
+@Setter
+private HealthCheckMetrics healthChecker;
Review comment:
As this is required in the process, please don't use the `setter`.
Initial this in the constructor.
##
File path:
oap-server/server-cluster-plugin/cluster-etcd-plugin/src/main/java/org/apache/skywalking/oap/server/cluster/plugin/etcd/EtcdCoordinator.java
##
@@ -73,15 +83,30 @@ public EtcdCoordinator(ClusterModuleEtcdConfig config,
EtcdClient client) {
if (!address.equals(selfAddress)) {
address.setSelf(false);
}
-res.add(new RemoteInstance(address));
+remoteInstances.add(new RemoteInstance(address));
});
}
-
-} catch (Exception e) {
+if (remoteInstances.size() > 1) {
+Set remoteAddressSet =
remoteInstances.stream().map(remoteInstance ->
+
remoteInstance.getAddress().getHost()).collect(Collectors.toSet());
+boolean hasUnHealthAddress =
OAPNodeChecker.hasUnHealthAddress(remoteAddressSet);
+if (hasUnHealthAddress) {
+this.healthChecker.unHealth(new
ServiceQueryException("found 127.0.0.1 or localhost in cluster mode"));
+} else {
+List selfInstances =
remoteInstances.stream().
+filter(remoteInstance ->
remoteInstance.getAddress().isSelf()).collect(Collectors.toList());
+if (CollectionUtils.isNotEmpty(selfInstances) &&
selfInstances.size() == 1) {
Review comment:
Why `&& selfInstances.size() == 1`? Are you planning to check duplicate
addresses? If so, please move this login into `OAPNodeChecker`, all coordinator
implementations should check this too, right?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a change in pull request #5661: Add health checker for cluster management module
wu-sheng commented on a change in pull request #5661:
URL: https://github.com/apache/skywalking/pull/5661#discussion_r506888747
##
File path:
oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/HealthCheckUtil.java
##
@@ -0,0 +1,34 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import com.google.common.collect.Sets;
+
+import java.util.Set;
+
+public class HealthCheckUtil {
Review comment:
Please move this class to the same folder of `ClusterModule`, as where
it belongs. Then rename it to `OAPNodeChecker`.
##
File path:
oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/HealthCheckUtil.java
##
@@ -0,0 +1,34 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import com.google.common.collect.Sets;
+
+import java.util.Set;
+
+public class HealthCheckUtil {
+private static final Set UN_HEALTH_ADDRESS =
Sets.newHashSet("127.0.0.1", "localhost");
Review comment:
```suggestion
private static final Set ILLEGAL_NODE_ADDRESS_IN_CLUSTER_MODE =
Sets.newHashSet("127.0.0.1", "localhost");
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a change in pull request #5661: Add health checker for cluster management module
wu-sheng commented on a change in pull request #5661:
URL: https://github.com/apache/skywalking/pull/5661#discussion_r506521308
##
File path:
oap-server/server-cluster-plugin/cluster-consul-plugin/src/main/java/org/apache/skywalking/oap/server/cluster/plugin/consul/ConsulCoordinator.java
##
@@ -49,22 +56,32 @@ public ConsulCoordinator(ClusterModuleConsulConfig config,
Consul client) {
@Override
public List queryRemoteNodes() {
-HealthClient healthClient = client.healthClient();
-
-// Discover only "passing" nodes
-List nodes =
healthClient.getHealthyServiceInstances(serviceName).getResponse();
-
List remoteInstances = new ArrayList<>();
-if (CollectionUtils.isNotEmpty(nodes)) {
-nodes.forEach(node -> {
-if (!Strings.isNullOrEmpty(node.getService().getAddress())) {
-Address address = new
Address(node.getService().getAddress(), node.getService().getPort(), false);
-if (address.equals(selfAddress)) {
-address.setSelf(true);
+try {
+HealthClient healthClient = client.healthClient();
+// Discover only "passing" nodes
+List nodes =
healthClient.getHealthyServiceInstances(serviceName).getResponse();
+if (CollectionUtils.isNotEmpty(nodes)) {
+nodes.forEach(node -> {
+if
(!Strings.isNullOrEmpty(node.getService().getAddress())) {
+Address address = new
Address(node.getService().getAddress(), node.getService().getPort(), false);
+if (address.equals(selfAddress)) {
+address.setSelf(true);
+}
+remoteInstances.add(new RemoteInstance(address));
Review comment:
Same thing should be checked in every coordinator implementation. I
noted that the users usually set up the cluster mode incorrectly, then, blame
SkyWalking insert duplicated data or statistic not accurate.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a change in pull request #5661: Add health checker for cluster management module
wu-sheng commented on a change in pull request #5661:
URL: https://github.com/apache/skywalking/pull/5661#discussion_r506519863
##
File path:
oap-server/server-cluster-plugin/cluster-consul-plugin/src/main/java/org/apache/skywalking/oap/server/cluster/plugin/consul/ConsulCoordinator.java
##
@@ -49,22 +56,32 @@ public ConsulCoordinator(ClusterModuleConsulConfig config,
Consul client) {
@Override
public List queryRemoteNodes() {
-HealthClient healthClient = client.healthClient();
-
-// Discover only "passing" nodes
-List nodes =
healthClient.getHealthyServiceInstances(serviceName).getResponse();
-
List remoteInstances = new ArrayList<>();
-if (CollectionUtils.isNotEmpty(nodes)) {
-nodes.forEach(node -> {
-if (!Strings.isNullOrEmpty(node.getService().getAddress())) {
-Address address = new
Address(node.getService().getAddress(), node.getService().getPort(), false);
-if (address.equals(selfAddress)) {
-address.setSelf(true);
+try {
+HealthClient healthClient = client.healthClient();
+// Discover only "passing" nodes
+List nodes =
healthClient.getHealthyServiceInstances(serviceName).getResponse();
+if (CollectionUtils.isNotEmpty(nodes)) {
+nodes.forEach(node -> {
+if
(!Strings.isNullOrEmpty(node.getService().getAddress())) {
+Address address = new
Address(node.getService().getAddress(), node.getService().getPort(), false);
+if (address.equals(selfAddress)) {
+address.setSelf(true);
+}
+remoteInstances.add(new RemoteInstance(address));
Review comment:
If you found `127.0.0.1` or `localhost` exists in the node
list(list#size > 1), then the node should be labeled as unhealthy.
FYI @hanahmily @kezhenxu94 Make sense?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a change in pull request #5661: Add health checker for cluster management module
wu-sheng commented on a change in pull request #5661:
URL: https://github.com/apache/skywalking/pull/5661#discussion_r506231634
##
File path:
oap-server/server-cluster-plugin/cluster-consul-plugin/src/main/java/org/apache/skywalking/oap/server/cluster/plugin/consul/ConsulCoordinator.java
##
@@ -27,19 +27,25 @@
import com.orbitz.consul.model.health.ServiceHealth;
import java.util.ArrayList;
import java.util.List;
+import java.util.stream.Collectors;
+
import org.apache.skywalking.oap.server.core.cluster.ClusterNodesQuery;
import org.apache.skywalking.oap.server.core.cluster.ClusterRegister;
import org.apache.skywalking.oap.server.core.cluster.RemoteInstance;
+import org.apache.skywalking.oap.server.core.cluster.ServiceQueryException;
import org.apache.skywalking.oap.server.core.cluster.ServiceRegisterException;
import org.apache.skywalking.oap.server.core.remote.client.Address;
+import
org.apache.skywalking.oap.server.library.client.healthcheck.HealthCheckable;
import org.apache.skywalking.oap.server.library.util.CollectionUtils;
+import org.apache.skywalking.oap.server.library.util.HealthChecker;
-public class ConsulCoordinator implements ClusterRegister, ClusterNodesQuery {
+public class ConsulCoordinator implements ClusterRegister, ClusterNodesQuery,
HealthCheckable {
Review comment:
You are still using the `
org.apache.skywalking.oap.server.library.util.HealthChecker` and
`org.apache.skywalking.oap.server.library.client.healthcheck.HealthCheckable`.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a change in pull request #5661: Add health checker for cluster management module
wu-sheng commented on a change in pull request #5661:
URL: https://github.com/apache/skywalking/pull/5661#discussion_r505387681
##
File path: oap-server/server-cluster-plugin/pom.xml
##
@@ -47,5 +47,10 @@
library-util
${project.version}
+
+org.apache.skywalking
+library-client
+${project.version}
+
Review comment:
You should remove this dependency totally.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
