zrlw commented on a change in pull request #9033:
URL: https://github.com/apache/dubbo/pull/9033#discussion_r727934788
##########
File path:
dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/deploy/DefaultModuleDeployer.java
##########
@@ -219,7 +226,9 @@ private void onModuleStarting() {
private void onModuleStarted(CompletableFuture startFuture) {
setStarted();
logger.info(getIdentifier() + " has started.");
- applicationDeployer.checkStarted(startFuture);
+ applicationDeployer.checkStarted();
+ // complete module start future after application state changed, fix
#9012 ?
+ startFuture.complete(true);
Review comment:
no. your applicationDeployer.checkStarted() only set starting flag if
one module is starting and return directly, it doesn't mean all module started
after applicationDeployer.checkStarted().
##########
File path: dubbo-common/src/main/java/org/apache/dubbo/rpc/model/ScopeModel.java
##########
@@ -69,7 +69,6 @@
private Map<String, Object> attributes;
private AtomicBoolean destroyed = new AtomicBoolean(false);
- private volatile boolean stopping;
Review comment:
see #9000
##########
File path:
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/metadata/store/InMemoryWritableMetadataService.java
##########
@@ -324,7 +328,7 @@ public void blockUntilUpdated() {
metadataSemaphore.drainPermits();
updateLock.writeLock().lock();
} catch (InterruptedException e) {
- if (!applicationModel.isStopping()) {
+ if (!applicationModel.isDestroyed()) {
Review comment:
AlbumenJ said in #9001:
Metadata refresh future should be canceled after unregister service
instance, which should be called before applicationModel.destroy().
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -83,7 +82,9 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- zkClient.close();
+ // TODO zkClient is shared in framework, should not close it here?
+ // zkClient.close();
Review comment:
see #8993
the send thread of zookeeper ClientCnxn will keep trying reconnect to the
closed zk Server if you don't close the zk client and it will cause more
integration testing problems.
##########
File path:
dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/deploy/DefaultModuleDeployer.java
##########
@@ -219,7 +226,9 @@ private void onModuleStarting() {
private void onModuleStarted(CompletableFuture startFuture) {
setStarted();
logger.info(getIdentifier() + " has started.");
- applicationDeployer.checkStarted(startFuture);
+ applicationDeployer.checkStarted();
+ // complete module start future after application state changed, fix
#9012 ?
+ startFuture.complete(true);
Review comment:
no. your applicationDeployer.checkStarted() only set starting flag and
return directly if one module is starting, it doesn't mean all module started
after applicationDeployer.checkStarted().
##########
File path: dubbo-common/src/main/java/org/apache/dubbo/rpc/model/ScopeModel.java
##########
@@ -69,7 +69,6 @@
private Map<String, Object> attributes;
private AtomicBoolean destroyed = new AtomicBoolean(false);
- private volatile boolean stopping;
Review comment:
see AlbumenJ said in #9001: Metadata refresh future should be canceled
after unregister service instance, which should be called before
applicationModel.destroy().
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -83,7 +82,9 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- zkClient.close();
+ // TODO zkClient is shared in framework, should not close it here?
+ // zkClient.close();
Review comment:
see #8993
the send thread of zookeeper ClientCnxn will keep trying reconnect to the
closed zk Server if you don't close the zk client and it will cause more
integration testing problems because huge reconnect failed events will block
curator global event loop processing.
##########
File path:
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/metadata/store/InMemoryWritableMetadataService.java
##########
@@ -324,7 +328,7 @@ public void blockUntilUpdated() {
metadataSemaphore.drainPermits();
updateLock.writeLock().lock();
} catch (InterruptedException e) {
- if (!applicationModel.isStopping()) {
+ if (!applicationModel.isDestroyed()) {
Review comment:
@AlbumenJ
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -83,7 +82,9 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- zkClient.close();
+ // TODO zkClient is shared in framework, should not close it here?
+ // zkClient.close();
Review comment:
using registry only as config center testing has no registry,
ZookeeperTransporter destroy() will not be called.
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -83,7 +82,9 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- zkClient.close();
+ // TODO zkClient is shared in framework, should not close it here?
+ // zkClient.close();
Review comment:
if SingleRegistryCenterInjvmIntegrationTest (using registry only as
config center testing has no registry) closes zkClient eventually then it will
be ok.
##########
File path:
dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/deploy/DefaultModuleDeployer.java
##########
@@ -219,7 +226,9 @@ private void onModuleStarting() {
private void onModuleStarted(CompletableFuture startFuture) {
setStarted();
logger.info(getIdentifier() + " has started.");
- applicationDeployer.checkStarted(startFuture);
+ applicationDeployer.checkStarted();
+ // complete module start future after application state changed, fix
#9012 ?
+ startFuture.complete(true);
Review comment:
set breakpoint at checkStarted() and
DubboBootstrapMultiInstanceTest.testMultiModuleDeployAndReload line 309,
debug DubboBootstrapMultiInstanceTest.testMultiModuleDeployAndReload as juit
test, you will see checkStarted will be called twice.
at the first time, if you let main thread continue, the test will be failed.
##########
File path:
dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/deploy/DefaultModuleDeployer.java
##########
@@ -219,7 +226,9 @@ private void onModuleStarting() {
private void onModuleStarted(CompletableFuture startFuture) {
setStarted();
logger.info(getIdentifier() + " has started.");
- applicationDeployer.checkStarted(startFuture);
+ applicationDeployer.checkStarted();
+ // complete module start future after application state changed, fix
#9012 ?
+ startFuture.complete(true);
Review comment:
set breakpoint at checkStarted() and
DubboBootstrapMultiInstanceTest.testMultiModuleDeployAndReload line 306:
```
moduleDeployer1.start().get();
```
debug DubboBootstrapMultiInstanceTest.testMultiModuleDeployAndReload as juit
test, you will see checkStarted will be called twice.
at the first time, if you let main thread continue, the test will be failed.
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -83,7 +82,9 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- zkClient.close();
+ // TODO zkClient is shared in framework, should not close it here?
+ // zkClient.close();
Review comment:
if SingleRegistryCenterInjvmIntegrationTest (using registry only as
config center testing has no registry) closes zkClient eventually then it is ok
for zookeeper client, but it's just one kind of dynamic configuration.
i think the root cause is dynamic configurations which need to be closed are
not to be closed.
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -83,7 +82,9 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- zkClient.close();
+ // TODO zkClient is shared in framework, should not close it here?
+ // zkClient.close();
Review comment:
if SingleRegistryCenterInjvmIntegrationTest (using registry only as
config center testing has no registry) closes zkClient eventually then it is ok
for zookeeper client, but it's just one kind of dynamic configuration.
i think the root cause is dynamic configurations which need to be closed are
not to be closed by now.
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -83,7 +82,9 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- zkClient.close();
+ // TODO zkClient is shared in framework, should not close it here?
+ // zkClient.close();
Review comment:
i tested SingleRegistryCenterInjvmIntegrationTest (using registry only
as config center testing has no registry) and zkClient was closed eventually,
so the fix is ok for zookeeper client, but zkClient is just one kind of dynamic
configuration which are not closed by now.
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -83,7 +82,9 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- zkClient.close();
+ // TODO zkClient is shared in framework, should not close it here?
+ // zkClient.close();
Review comment:
let's say we have a two application A and B. A connect zk A, B connect
zk B, A is short life app which only run 1 minute, the zkClient of A should be
closed at A exited.
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -83,7 +82,9 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- zkClient.close();
+ // TODO zkClient is shared in framework, should not close it here?
+ // zkClient.close();
Review comment:
let's say we have a two application A and B. A connect zk A, B connect
zk B, A is short life app which only run 1 minute, the zkClient of A should be
closed at A exited, otherwise it still cause reconnect ion trying problem.
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -83,7 +82,9 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- zkClient.close();
+ // TODO zkClient is shared in framework, should not close it here?
+ // zkClient.close();
Review comment:
let's say we have a two application A and B. A connect zk A, B connect
zk B, A is short life app which only run 1 minute, the zkClient of A should be
closed at A exited, otherwise it still cause reconnect ion trying problem.
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -83,7 +82,9 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- zkClient.close();
+ // TODO zkClient is shared in framework, should not close it here?
+ // zkClient.close();
Review comment:
let's say we have a two application A and B. A connect zk A, B connect
zk B, A is short life app which only run 1 minute, the zkClient of A should be
closed at A exited, otherwise it still cause reconnect ion trying problem.
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -83,7 +82,9 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- zkClient.close();
+ // TODO zkClient is shared in framework, should not close it here?
+ // zkClient.close();
Review comment:
let's say we have two application A and B. A connect zk A, B connect zk
B, A is short life app which only run 1 minute, the zkClient of A should be
closed when A exited, otherwise it still cause curator reconnection failure
events surge problem.
##########
File path:
dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java
##########
@@ -82,8 +82,10 @@ public String getInternalProperty(String key) {
@Override
protected void doClose() throws Exception {
- // TODO zkClient is shared in framework, should not close it here?
+ // zkClient is shared in framework, should not close it here
Review comment:
keeping zkClient independent in each application might be better，not all
app connect same registry and have same lifecycle.
#9015
--
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]
