kylixs commented on a change in pull request #9033:
URL: https://github.com/apache/dubbo/pull/9033#discussion_r727950147
##########
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:
If the ApplicationModel is stopping, the value of `isDestroyed() ` is
true, no need to add a `stopping` field. Furthermore, I want to add a `state`
field to `ApplicationModel` and `ModuleModel`.
##########
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 here:
org.apache.dubbo.remoting.zookeeper.AbstractZookeeperTransporter#destroy()
All zk clients is created and destroyed in ZookeeperTransporter.
```java
@Override
public void destroy() {
// only destroy zk clients here
for (ZookeeperClient client : zookeeperClientMap.values()) {
client.close();
}
zookeeperClientMap.clear();
}
```
##########
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:
The entry of destroy an application is `ApplicationModel.destroy()`.
So, check destroyed is ok.
##########
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:
The `ZookeeperTransporter` is shared in framework scope, so zk clients
is shared in framework, cannot be destroyed when application is shutdown.
##########
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:
The `ZookeeperTransporter` is shared in framework scope, so zk clients
is shared in framework, cannot be destroyed when one application is shutdown
but some application is alive .
##########
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:
The `DefaultApplicationDeployer.checkStarted()` function is used to
check all modules state and notify start checking.
If start by module manually in some scenario, maybe some modules are
started, but the application is starting until all modules are started.
```java
private void doStart() {
startModules();
// prepare application instance
prepareApplicationInstance();
executorRepository.getSharedExecutor().submit(() -> {
while (true) {
// notify on each module started
synchronized (startedLock) {
try {
startedLock.wait(500);
} catch (InterruptedException e) {
// ignore
}
}
// if has new module, do start again
if (hasPendingModule()) {
startModules();
continue;
}
DeployState state = checkState();
if (!(state == DeployState.STARTING || state ==
DeployState.PENDING)) {
// start finished or error
break;
}
}
});
}
....
public void checkStarted() {
// TODO improve state checking
DeployState _state = checkState();
switch (_state) {
case STARTED:
onStarted();
break;
case STARTING:
onStarting();
break;
case PENDING:
setPending();
break;
}
// notify started
synchronized (startedLock) {
startedLock.notifyAll();
}
}
```
##########
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:
The `startFuture` of `DefaultModuleDeployer` just monitors the start
action of the module, and has no strong relationship with the application.
--
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]
