重构 ServiceBean 的 isDelay 方法，使其更符合语义

[code4wt]

isDelay 方法目前返回值如下：

• true: delay == null || delay == -1，表示不延迟导出服务
• false: delay > 0，表示延迟导出服务

我觉得 isDelay 方法返回值如下时，更合理一些：

• true: 延迟导出服务
• false: 无需延迟导出服务

What is the purpose of the change

Fix the wrong meaning of the method isDelay

[codecov-io]

Codecov Report

Merging #2686 into master will increase coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2686      +/-   ##
==========================================
+ Coverage   63.24%   63.29%   +0.04%     
==========================================
  Files         573      578       +5     
  Lines       25763    25958     +195     
  Branches     4533     4545      +12     
==========================================
+ Hits        16294    16430     +136     
- Misses       7320     7365      +45     
- Partials     2149     2163      +14

Impacted Files	Coverage Δ
...va/org/apache/dubbo/config/spring/ServiceBean.java	47.65% <0%> (-2.72%)	⬇️
...a/org/apache/dubbo/rpc/filter/AccessLogFilter.java	52.22% <0%> (-18.89%)	⬇️
...ache/dubbo/remoting/transport/AbstractChannel.java	75% <0%> (-12.5%)	⬇️
...org/apache/dubbo/rpc/filter/ActiveLimitFilter.java	77.77% <0%> (-11.12%)	⬇️
...rpc/cluster/loadbalance/RoundRobinLoadBalance.java	85.29% <0%> (-8.46%)	⬇️
...e/dubbo/remoting/transport/netty/NettyChannel.java	54.11% <0%> (-8.24%)	⬇️
.../apache/dubbo/remoting/transport/AbstractPeer.java	63.04% <0%> (-4.35%)	⬇️
...rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java	54.21% <0%> (-3.62%)	⬇️
...he/dubbo/remoting/transport/netty/NettyServer.java	67.85% <0%> (-3.58%)	⬇️
...org/apache/dubbo/rpc/protocol/AbstractInvoker.java	59.67% <0%> (-3.23%)	⬇️
... and 22 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7286e23...2f1b767. Read the comment docs.

[beiwei30]

@code4wt In fact, I believe isDelay() is unnecessary any longer in ServiceBean. Here's the reason, the method export in ServiceConfig should have already covered this logic:

    public synchronized void export() {
        if (provider != null) {
            if (export == null) {
                export = provider.getExport();
            }
            if (delay == null) {
                delay = provider.getDelay();
            }
        }
        if (export != null && !export) {
            return;
        }

        if (delay != null && delay > 0) {
            delayExportExecutor.schedule(new Runnable() {
                @Override
                public void run() {
                    doExport();
                }
            }, delay, TimeUnit.MILLISECONDS);
        } else {
            doExport();
        }
    }

But I agree we need to enhance ServiceBean since the current logic is confusing, what I propose is:

@@ -107,7 +107,7 @@ public class ServiceBean<T> extends ServiceConfig<T> implements InitializingBean
 
     @Override
     public void onApplicationEvent(ContextRefreshedEvent event) {
-        if (isDelay() && !isExported() && !isUnexported()) {
+        if (!isExported() && !isUnexported()) {^M
             if (logger.isInfoEnabled()) {
                 logger.info("The service ready on spring started. service: " + getInterface());
             }
@@ -115,15 +115,6 @@ public class ServiceBean<T> extends ServiceConfig<T> implements InitializingBean
         }
     }
 
-    private boolean isDelay() {
-        Integer delay = getDelay();
-        ProviderConfig provider = getProvider();
-        if (delay == null && provider != null) {
-            delay = provider.getDelay();
-        }
-        return supportedApplicationListener && (delay == null || delay == -1);
-    }
-
     @Override
     @SuppressWarnings({"unchecked", "deprecation"})
     public void afterPropertiesSet() throws Exception {
@@ -251,7 +242,7 @@ public class ServiceBean<T> extends ServiceConfig<T> implements InitializingBean
                 setPath(beanName);
             }
         }
-        if (!isDelay()) {
+        if (!supportedApplicationListener) {^M
             export();
         }
     }

Pls. take a look. If you agree with this, would you mind to revise your pull request?

[code4wt]

@code4wt In fact, I believe isDelay() is unnecessary any longer in ServiceBean. Here's the reason, the method export in ServiceConfig should have already covered this logic:

    public synchronized void export() {
        if (provider != null) {
            if (export == null) {
                export = provider.getExport();
            }
            if (delay == null) {
                delay = provider.getDelay();
            }
        }
        if (export != null && !export) {
            return;
        }

        if (delay != null && delay > 0) {
            delayExportExecutor.schedule(new Runnable() {
                @Override
                public void run() {
                    doExport();
                }
            }, delay, TimeUnit.MILLISECONDS);
        } else {
            doExport();
        }
    }

But I agree we need to enhance ServiceBean since the current logic is confusing, what I propose is:

@@ -107,7 +107,7 @@ public class ServiceBean<T> extends ServiceConfig<T> implements InitializingBean
 
     @Override
     public void onApplicationEvent(ContextRefreshedEvent event) {
-        if (isDelay() && !isExported() && !isUnexported()) {
+        if (!isExported() && !isUnexported()) {^M
             if (logger.isInfoEnabled()) {
                 logger.info("The service ready on spring started. service: " + getInterface());
             }
@@ -115,15 +115,6 @@ public class ServiceBean<T> extends ServiceConfig<T> implements InitializingBean
         }
     }
 
-    private boolean isDelay() {
-        Integer delay = getDelay();
-        ProviderConfig provider = getProvider();
-        if (delay == null && provider != null) {
-            delay = provider.getDelay();
-        }
-        return supportedApplicationListener && (delay == null || delay == -1);
-    }
-
     @Override
     @SuppressWarnings({"unchecked", "deprecation"})
     public void afterPropertiesSet() throws Exception {
@@ -251,7 +242,7 @@ public class ServiceBean<T> extends ServiceConfig<T> implements InitializingBean
                 setPath(beanName);
             }
         }
-        if (!isDelay()) {
+        if (!supportedApplicationListener) {^M
             export();
         }
     }

Pls. take a look. If you agree with this, would you mind to revise your pull request?

Thanks to review this PR. I agree with you, isDelay really is an unnecessary method. I have revised my PR , pls check.

[beiwei30]

good job, thanks.

[zonghaishang]

What if you want to expose the service 10 seconds after the spring container started ?
The original intention delay = 10000 represents execution after 10 seconds.

@beiwei30 @code4wt @chickenlj please help check that.

public synchronized void export() {
    if (delay != null && delay > 0) {
        // 
        delayExportExecutor.schedule(new Runnable() {
            @Override
            public void run() {
                doExport();
            }
        }, delay, TimeUnit.MILLISECONDS);
    } else {
        doExport();
    }
}

[beiwei30]

I think this behavior is expected. The reason is, the best timing for Dubbo to count down delay for exposing service is when Spring container is ready, then this timing should be the moment when spring context gets refreshed, since at this moment, all beans are settled, and are ready for wiring in their dependencies. If it happens earlier, say, at the moment after properties set, then Dubbo may fail to expose service because it depends on other spring bean.

To conclude, Dubbo should start to expose services until spring context gets refreshed, no matter delay or no-delay is configured. Unfortunately in order to keep backward compatibility, considering not all spring versions support context refresh callback, we have to start to expose services after properties set if we find application listener is not supported, and hope Dubbo service can be exposed normally.

I hope this can clarify the behavior.

[jarvisxiong]

赞，最近看这块有些别扭。

[LuckyDevin]

very much like

[574258039]

very much like

[Alexbonbon]

看着真是别扭

[iupikachu]

ok