New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

重构 ServiceBean 的 isDelay 方法,使其更符合语义 #2686

Merged
merged 2 commits into from Oct 30, 2018

Conversation

Projects
None yet
5 participants
@code4wt
Contributor

code4wt commented Oct 25, 2018

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

@code4wt code4wt changed the title from 重构 ServiceBean 的 isDelay,使其更符合语义 to 重构 ServiceBean 的 isDelay 方法,使其更符合语义 Oct 25, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Oct 25, 2018

Codecov Report

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

Impacted file tree graph

@@            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

This comment has been minimized.

Contributor

beiwei30 commented Oct 30, 2018

@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

This comment has been minimized.

Contributor

code4wt commented Oct 30, 2018

@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

This comment has been minimized.

Contributor

beiwei30 commented Oct 30, 2018

good job, thanks.

@beiwei30 beiwei30 merged commit 4d0a36c into apache:master Oct 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zonghaishang

This comment has been minimized.

Member

zonghaishang commented Oct 31, 2018

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

This comment has been minimized.

Contributor

beiwei30 commented Nov 1, 2018

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

This comment has been minimized.

jarvisxiong commented Nov 8, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment