pinxiong commented on a change in pull request #9138:
URL: https://github.com/apache/dubbo/pull/9138#discussion_r743359356
##########
File path:
dubbo-test/dubbo-test-check/src/main/resources/META-INF/services/org.junit.platform.launcher.TestExecutionListener
##########
@@ -1 +1,5 @@
+# DubboRegistryCenterStarted should be the first one because of the startup of
zookeeper
+org.apache.dubbo.test.check.RegistryCenterStarted
Review comment:
> Pls add JVM arg to disable embedded zk server, allow users to use
external zk server.
It's done.
##########
File path:
dubbo-test/dubbo-test-check/src/main/resources/META-INF/services/org.junit.platform.launcher.TestExecutionListener
##########
@@ -1 +1,5 @@
+# DubboRegistryCenterStarted should be the first one because of the startup of
zookeeper
+org.apache.dubbo.test.check.RegistryCenterStarted
Review comment:
> When enable thread checking by `-DcheckThreads=true`,
`DubboTestChecker` will dump threads after finished each test class. The
embedded zk server will also create some threads, such as creating a new netty
thread when establishing a connection, which will interfere with the thread
check of dubbo test. Is it possible to start zk server in a separate process?
Now, it can use a dependent process to start zookeeper server.
##########
File path:
dubbo-test/dubbo-test-check/src/main/java/org/apache/dubbo/test/check/registrycenter/processor/KillProcessWindowsProcessor.java
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.dubbo.test.check.registrycenter.processor;
+
+import org.apache.commons.exec.CommandLine;
+import org.apache.commons.exec.DefaultExecutor;
+import org.apache.commons.exec.Executor;
+import org.apache.commons.exec.PumpStreamHandler;
+import org.apache.dubbo.common.logger.Logger;
+import org.apache.dubbo.common.logger.LoggerFactory;
+import org.apache.dubbo.test.check.exception.DubboTestException;
+import
org.apache.dubbo.test.check.registrycenter.context.ZookeeperWindowsContext;
+
+import java.io.IOException;
+
+/**
+ * Create a {@link org.apache.dubbo.test.check.registrycenter.Processor} to
kill pid on Windows OS.
+ */
+public class KillProcessWindowsProcessor extends ZookeeperWindowsProcessor {
+
+ private static final Logger logger =
LoggerFactory.getLogger(KillProcessWindowsProcessor.class);
+
+ @Override
+ protected void doProcess(ZookeeperWindowsContext context) throws
DubboTestException {
+ for (int clientPort : context.getClientPorts()) {
+ Integer pid = context.getPid(clientPort);
Review comment:
OK, I've removed the killed pid.
##########
File path:
dubbo-config/dubbo-config-api/src/test/java/org/apache/dubbo/config/integration/multiple/exportmetadata/MultipleRegistryCenterExportMetadataIntegrationTest.java
##########
@@ -99,13 +91,9 @@ public void setUp() throws Exception {
DubboBootstrap.getInstance()
.application(new ApplicationConfig(PROVIDER_APPLICATION_NAME))
.protocol(new ProtocolConfig(PROTOCOL_NAME))
- .service(serviceConfig);
- for (RegistryCenter.Instance instance :
registryCenter.getRegistryCenterInstance()) {
- DubboBootstrap.getInstance().registry(new
RegistryConfig(String.format("%s://%s:%s",
- instance.getType(),
- instance.getHostname(),
- instance.getPort())));
- }
+ .service(serviceConfig)
+ .registry(new RegistryConfig("zookeeper://127.0.0.1:2181"))
+ .registry(new RegistryConfig("zookeeper://127.0.0.1:2182"));
Review comment:
That's a good seggustion! However, all local `zooKeeper connection
strings` aren't displayed clearly, especially using Spring configuration or
annotation.
But I've supported this feature.
##########
File path:
dubbo-config/dubbo-config-api/src/test/java/org/apache/dubbo/config/integration/multiple/exportmetadata/MultipleRegistryCenterExportMetadataIntegrationTest.java
##########
@@ -99,13 +91,9 @@ public void setUp() throws Exception {
DubboBootstrap.getInstance()
.application(new ApplicationConfig(PROVIDER_APPLICATION_NAME))
.protocol(new ProtocolConfig(PROTOCOL_NAME))
- .service(serviceConfig);
- for (RegistryCenter.Instance instance :
registryCenter.getRegistryCenterInstance()) {
- DubboBootstrap.getInstance().registry(new
RegistryConfig(String.format("%s://%s:%s",
- instance.getType(),
- instance.getHostname(),
- instance.getPort())));
- }
+ .service(serviceConfig)
+ .registry(new RegistryConfig("zookeeper://127.0.0.1:2181"))
+ .registry(new RegistryConfig("zookeeper://127.0.0.1:2182"));
Review comment:
The best way is to define two client ports (which might not be `2181`
and `2182`) and quote them when using. However, there are so many hard code in
unit test. So, it's very diffcult to change anywhere.
such as some zookeeper connection string in properties file
```java
dubbo.registry.address=zookeeper://127.0.0.1:2181?registry-type=service
biz.group=greeting
biz.group2=group2
dubbo.call-timeout=2000
```
##########
File path:
dubbo-test/dubbo-test-check/src/main/java/org/apache/dubbo/test/check/registrycenter/MockedRegistryCenter.java
##########
@@ -21,6 +21,16 @@
*/
public class MockedRegistryCenter {
+ /**
+ * The first global zookeeper address
+ */
+ public static final String ZOOKEEPER_ADDRESS1 =
"zookeeper://127.0.0.1:2181";
Review comment:
> The global zookeeper address configs should not mixed with embedded zk
server, extract them into a outer class. Furthermore, I thank it's better to
append the zk server address and port configs to system props, such as:
>
> ```
> zookeeper.address=zookeeper://127.0.0.1:2181
> zookeeper.port=2181
> zookeeper.address.1=zookeeper://127.0.0.1:2181
> zookeeper.port.1=2181
> zookeeper.address.2=zookeeper://127.0.0.1:2182
> zookeeper.port.2=2182
> ```
>
> So we can use them in Spring application.properties/yaml:
>
> ```
> dubbo.registry.address=${zookeeper.address}
> dubbo.registries.second-registry.address=${zookeeper.address.2}
> ```
I think we need to support that make `zookeeper port` as system props,
don't do that for `zookeeper.address`, because the `zookeeper.address` depend
on `zookeeper.port`
For `zookeeper.address`, we can set the system props config after zookeeper
instance started.
##########
File path:
dubbo-test/dubbo-test-check/src/main/resources/META-INF/services/org.junit.platform.launcher.TestExecutionListener
##########
@@ -1 +1,5 @@
+# DubboRegistryCenterStarted should be the first one because of the startup of
zookeeper
+org.apache.dubbo.test.check.RegistryCenterStarted
Review comment:
> Pls add JVM arg to disable embedded zk server, allow users to use
external zk server.
It's done.
##########
File path:
dubbo-test/dubbo-test-check/src/main/resources/META-INF/services/org.junit.platform.launcher.TestExecutionListener
##########
@@ -1 +1,5 @@
+# DubboRegistryCenterStarted should be the first one because of the startup of
zookeeper
+org.apache.dubbo.test.check.RegistryCenterStarted
Review comment:
> When enable thread checking by `-DcheckThreads=true`,
`DubboTestChecker` will dump threads after finished each test class. The
embedded zk server will also create some threads, such as creating a new netty
thread when establishing a connection, which will interfere with the thread
check of dubbo test. Is it possible to start zk server in a separate process?
Now, it can use a dependent process to start zookeeper server.
##########
File path:
dubbo-test/dubbo-test-check/src/main/java/org/apache/dubbo/test/check/registrycenter/processor/KillProcessWindowsProcessor.java
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.dubbo.test.check.registrycenter.processor;
+
+import org.apache.commons.exec.CommandLine;
+import org.apache.commons.exec.DefaultExecutor;
+import org.apache.commons.exec.Executor;
+import org.apache.commons.exec.PumpStreamHandler;
+import org.apache.dubbo.common.logger.Logger;
+import org.apache.dubbo.common.logger.LoggerFactory;
+import org.apache.dubbo.test.check.exception.DubboTestException;
+import
org.apache.dubbo.test.check.registrycenter.context.ZookeeperWindowsContext;
+
+import java.io.IOException;
+
+/**
+ * Create a {@link org.apache.dubbo.test.check.registrycenter.Processor} to
kill pid on Windows OS.
+ */
+public class KillProcessWindowsProcessor extends ZookeeperWindowsProcessor {
+
+ private static final Logger logger =
LoggerFactory.getLogger(KillProcessWindowsProcessor.class);
+
+ @Override
+ protected void doProcess(ZookeeperWindowsContext context) throws
DubboTestException {
+ for (int clientPort : context.getClientPorts()) {
+ Integer pid = context.getPid(clientPort);
Review comment:
OK, I've removed the killed pid.
##########
File path:
dubbo-config/dubbo-config-api/src/test/java/org/apache/dubbo/config/integration/multiple/exportmetadata/MultipleRegistryCenterExportMetadataIntegrationTest.java
##########
@@ -99,13 +91,9 @@ public void setUp() throws Exception {
DubboBootstrap.getInstance()
.application(new ApplicationConfig(PROVIDER_APPLICATION_NAME))
.protocol(new ProtocolConfig(PROTOCOL_NAME))
- .service(serviceConfig);
- for (RegistryCenter.Instance instance :
registryCenter.getRegistryCenterInstance()) {
- DubboBootstrap.getInstance().registry(new
RegistryConfig(String.format("%s://%s:%s",
- instance.getType(),
- instance.getHostname(),
- instance.getPort())));
- }
+ .service(serviceConfig)
+ .registry(new RegistryConfig("zookeeper://127.0.0.1:2181"))
+ .registry(new RegistryConfig("zookeeper://127.0.0.1:2182"));
Review comment:
That's a good seggustion! However, all local `zooKeeper connection
strings` aren't displayed clearly, especially using Spring configuration or
annotation.
But I've supported this feature.
##########
File path:
dubbo-config/dubbo-config-api/src/test/java/org/apache/dubbo/config/integration/multiple/exportmetadata/MultipleRegistryCenterExportMetadataIntegrationTest.java
##########
@@ -99,13 +91,9 @@ public void setUp() throws Exception {
DubboBootstrap.getInstance()
.application(new ApplicationConfig(PROVIDER_APPLICATION_NAME))
.protocol(new ProtocolConfig(PROTOCOL_NAME))
- .service(serviceConfig);
- for (RegistryCenter.Instance instance :
registryCenter.getRegistryCenterInstance()) {
- DubboBootstrap.getInstance().registry(new
RegistryConfig(String.format("%s://%s:%s",
- instance.getType(),
- instance.getHostname(),
- instance.getPort())));
- }
+ .service(serviceConfig)
+ .registry(new RegistryConfig("zookeeper://127.0.0.1:2181"))
+ .registry(new RegistryConfig("zookeeper://127.0.0.1:2182"));
Review comment:
The best way is to define two client ports (which might not be `2181`
and `2182`) and quote them when using. However, there are so many hard code in
unit test. So, it's very diffcult to change anywhere.
such as some zookeeper connection string in properties file
```java
dubbo.registry.address=zookeeper://127.0.0.1:2181?registry-type=service
biz.group=greeting
biz.group2=group2
dubbo.call-timeout=2000
```
##########
File path:
dubbo-test/dubbo-test-check/src/main/java/org/apache/dubbo/test/check/registrycenter/MockedRegistryCenter.java
##########
@@ -21,6 +21,16 @@
*/
public class MockedRegistryCenter {
+ /**
+ * The first global zookeeper address
+ */
+ public static final String ZOOKEEPER_ADDRESS1 =
"zookeeper://127.0.0.1:2181";
Review comment:
> The global zookeeper address configs should not mixed with embedded zk
server, extract them into a outer class. Furthermore, I thank it's better to
append the zk server address and port configs to system props, such as:
>
> ```
> zookeeper.address=zookeeper://127.0.0.1:2181
> zookeeper.port=2181
> zookeeper.address.1=zookeeper://127.0.0.1:2181
> zookeeper.port.1=2181
> zookeeper.address.2=zookeeper://127.0.0.1:2182
> zookeeper.port.2=2182
> ```
>
> So we can use them in Spring application.properties/yaml:
>
> ```
> dubbo.registry.address=${zookeeper.address}
> dubbo.registries.second-registry.address=${zookeeper.address.2}
> ```
I think we need to support that make `zookeeper port` as system props,
don't do that for `zookeeper.address`, because the `zookeeper.address` depend
on `zookeeper.port`
For `zookeeper.address`, we can set the system props config after zookeeper
instance started.
--
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]
