Skip to content
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

Make all the thread name start with SOFA #85

Merged
merged 8 commits into from
May 5, 2018
Merged

Make all the thread name start with SOFA #85

merged 8 commits into from
May 5, 2018

Conversation

liangyuanpeng
Copy link
Contributor

@liangyuanpeng liangyuanpeng commented May 4, 2018

all the thread name start with SOFA
detail see #78

@ujjboy
Copy link
Member

ujjboy commented May 4, 2018

@liangyuanpeng You need run mvn clean compile before you start new PR.
See https://github.com/alipay/sofa-rpc/wiki/Contributing.

@liangyuanpeng
Copy link
Contributor Author

liangyuanpeng commented May 4, 2018

hi,@ujjboy I've done this mvn clean compile
The reason for the failure is that "the command "sh ./tools/check_format.sh" failed and exited with 1 during "

@ujjboy
Copy link
Member

ujjboy commented May 4, 2018

No, I think you must did not run this before you commit code, otherwise It will not exists uncommit files. Just try again, and you will find NamedThreadFactory is not formatted.

@liangyuanpeng
Copy link
Contributor Author

ok,my fault
so how can i do now?
create a new pr?

@khotyn
Copy link
Member

khotyn commented May 4, 2018

@liangyuanpeng You don't have to create a new PR. You can just run mvn clean compile and commit all the reformat files and push to the origin branch of this PR, then all the check will be re-run automatically.

@ujjboy
Copy link
Member

ujjboy commented May 4, 2018

Just keep commit to your dev branch liangyuanpeng:enhancement_threadname_feature and CI will trigger a new build.

@liangyuanpeng
Copy link
Contributor Author

@khotyn @ujjboy got it , thank you for help!!!

@codecov-io
Copy link

codecov-io commented May 4, 2018

Codecov Report

Merging #85 into master will increase coverage by 0.05%.
The diff coverage is 90.9%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #85      +/-   ##
============================================
+ Coverage     63.84%   63.89%   +0.05%     
+ Complexity      596      595       -1     
============================================
  Files           263      263              
  Lines         11304    11304              
  Branches       1892     1892              
============================================
+ Hits           7217     7223       +6     
+ Misses         3114     3109       -5     
+ Partials        973      972       -1
Impacted Files Coverage Δ Complexity Δ
...java/com/alipay/sofa/rpc/context/AsyncRuntime.java 53.84% <100%> (ø) 0 <0> (ø) ⬇️
...pay/sofa/rpc/common/struct/NamedThreadFactory.java 81.25% <100%> (+1.25%) 0 <0> (ø) ⬇️
...va/com/alipay/sofa/rpc/server/bolt/BoltServer.java 76.62% <100%> (ø) 21 <0> (ø) ⬇️
...pay/sofa/rpc/server/rest/SofaNettyJaxrsServer.java 60.78% <100%> (ø) 14 <1> (ø) ⬇️
.../sofa/rpc/client/aft/impl/TimeWindowRegulator.java 81.91% <100%> (ø) 7 <0> (ø) ⬇️
...y/sofa/rpc/bootstrap/DefaultProviderBootstrap.java 60.91% <50%> (+0.81%) 0 <0> (ø) ⬇️
...ava/com/alipay/sofa/rpc/config/RegistryConfig.java 35% <0%> (-1%) 0% <0%> (ø)
.../alipay/sofa/rpc/registry/local/LocalRegistry.java 57.22% <0%> (-0.61%) 25% <0%> (-1%)
...com/alipay/sofa/rpc/context/RpcRuntimeContext.java 80.24% <0%> (+1.23%) 0% <0%> (ø) ⬇️
...n/java/com/alipay/sofa/rpc/common/SofaConfigs.java 85.71% <0%> (+1.78%) 0% <0%> (ø) ⬇️
... and 1 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 b6ba240...bb7e804. Read the comment docs.

*/
public NamedThreadFactory(String prefix) {
this(prefix, false);
public NamedThreadFactory(String prefix2) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the parameter name to secondPrefix? Because you have a field called firstPrefix, so I think secondPrefix is more suitable then prefix2

@ujjboy
Copy link
Member

ujjboy commented May 4, 2018

Hi, @liangyuanpeng!

The idea of automatically add prefix is good, but NamedThreadFactory will be used in UserThreadPool, if we add the first prefix automatically, the user will get different thread name with the name they expected.

So I suggest we don't change the behavior of NamedThreadFactory, just change the code which use UserThreadPool.

And you missed the files below:

  1. com.alipay.sofa.rpc.bootstrap.DefaultProviderBootstrap#export

    use code like:

    new NamedThreadFactory("SOFA-DELAY-EXPORT").newThread(new Runnable());
    
  2. com.alipay.sofa.rpc.server.bolt.BoltServer#initThreadPool

    new NamedThreadFactory("SOFA-BOLT-BIZ")
    

@liangyuanpeng
Copy link
Contributor Author

liangyuanpeng commented May 4, 2018

@ujjboy

I think start with "SOFA" just some like end with "-T"

public NamedThreadFactory(String prefix, boolean daemon) {
        SecurityManager s = System.getSecurityManager();
        group = (s != null) ? s.getThreadGroup() : Thread.currentThread().getThreadGroup();
        namePrefix = prefix + "-" + POOL_COUNT.getAndIncrement() + "-T";
        isDaemon = daemon;
    }

and in my code , have no “SOFA-DELAY-EXPORT” and “SOFA-BOLT-BIZ”

  1. com.alipay.sofa.rpc.bootstrap.DefaultProviderBootstrap#export
public void export() {
        if (providerConfig.getDelay() > 0) { // 延迟加载,单位毫秒
            Thread thread = new Thread(new Runnable() {
                @Override
                public void run() {
                    try {
                        Thread.sleep(providerConfig.getDelay());
                    } catch (Throwable ignore) { // NOPMD
                    }
                    doExport();
                }
            });
            thread.setDaemon(true);
            thread.setName("DelayExportThread");
            thread.start();
        } else {
            doExport();
        }
    }

2.com.alipay.sofa.rpc.server.bolt.BoltServer#initThreadPool

        threadPool.setThreadFactory(new NamedThreadFactory(
            "SofaBizProcessor-" + serverConfig.getPort(), serverConfig.isDaemon()));
        threadPool.setRejectedExecutionHandler(new SofaRejectedExecutionHandler());

@ujjboy
Copy link
Member

ujjboy commented May 4, 2018

@liangyuanpeng I means the code which you post need change to DELAY-EXPORT and BOLT-BIZ

And I found you miss more code blow:

com.alipay.sofa.rpc.client.aft.impl.TimeWindowRegulator#measureScheduler
com.alipay.sofa.rpc.client.aft.impl.TimeWindowRegulator#regulationExecutor

@liangyuanpeng
Copy link
Contributor Author

liangyuanpeng commented May 4, 2018

@ujjboy
do you mean some code like that

1.com.alipay.sofa.rpc.bootstrap.DefaultProviderBootstrap#export

Thread thread = new NamedThreadFactory("DELAY-EXPORT",true).newThread(new Runnable() {
                @Override
                public void run() {
                    try {
                        Thread.sleep(providerConfig.getDelay());
                    } catch (Throwable ignore) { // NOPMD
                    }
                    doExport();
                }
            });
            thread.start();

2.com.alipay.sofa.rpc.server.bolt.BoltServer#initThreadPool

ThreadPoolExecutor threadPool = BusinessPool.initPool(serverConfig);
        threadPool.setThreadFactory(new NamedThreadFactory(
            "BOLT-BIZ" + serverConfig.getPort(), serverConfig.isDaemon()));
        threadPool.setRejectedExecutionHandler(new SofaRejectedExecutionHandler());

@ujjboy
Copy link
Member

ujjboy commented May 4, 2018

@liangyuanpeng 👍 Yes, that is what I mean.

And new NamedThreadFactory("DELAY-EXPORT",true) can be a field, and then create as needed.

@@ -87,7 +87,7 @@ public void init(ServerConfig serverConfig) {
protected ThreadPoolExecutor initThreadPool(ServerConfig serverConfig) {
ThreadPoolExecutor threadPool = BusinessPool.initPool(serverConfig);
threadPool.setThreadFactory(new NamedThreadFactory(
"SofaBizProcessor-" + serverConfig.getPort(), serverConfig.isDaemon()));
"BOLT-BIZ" + serverConfig.getPort(), serverConfig.isDaemon()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe "BOLT-BIZ-" ?

@ujjboy ujjboy merged commit 3e8aab8 into sofastack:master May 5, 2018
@ujjboy ujjboy added the enhancement New feature or request label May 5, 2018
@ujjboy ujjboy added this to the 5.4.0 milestone May 5, 2018
@ujjboy ujjboy changed the title all the thread name start with SOFA Make all the thread name start with SOFA May 30, 2018
@liangyuanpeng liangyuanpeng deleted the enhancement_threadname_feature branch August 27, 2018 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants