Simple date format each time new object creation removed #3027
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3027 +/- ##
==========================================
- Coverage 64.24% 64.21% -0.04%
==========================================
Files 584 584
Lines 26056 26059 +3
Branches 4562 4562
==========================================
- Hits 16740 16734 -6
- Misses 7132 7139 +7
- Partials 2184 2186 +2
Continue to review full report at Codecov.
|
| } | ||
| }; | ||
|
|
||
| static ThreadLocal<SimpleDateFormat> MESSAGE_DATE_FORMATTER= new ThreadLocal<SimpleDateFormat>() { |
There was a problem hiding this comment.
Hi, khan
We have integrated Netty's FastThreadLocal in dubbo. I think we can use it instead of ThreadLocal. I suggest you refer to RpcContext.LOCAL.
There was a problem hiding this comment.
Thanks for suggesting this. I was looking into RpcContext code for LOCAL and SERVER_LOCAL implementation, I would like to go this approach.
I am very curious to know, the difference between LOCAL and SERVER_LOCAL here. Is LOCAL is used for when the call is from consumer and SERVER_LOCAL is used for,when call is served by SERVER? Is this understanding of mine is correct?
| @@ -113,7 +127,7 @@ public Result invoke(Invoker<?> invoker, Invocation inv) throws RpcException { | |||
| String version = invoker.getUrl().getParameter(Constants.VERSION_KEY); | |||
| String group = invoker.getUrl().getParameter(Constants.GROUP_KEY); | |||
| StringBuilder sn = new StringBuilder(); | |||
| sn.append("[").append(new SimpleDateFormat(MESSAGE_DATE_FORMAT).format(new Date())).append("] ").append(context.getRemoteHost()).append(":").append(context.getRemotePort()) | |||
| sn.append("[").append(MESSAGE_DATE_FORMATTER.get().format(new Date())).append("] ").append(context.getRemoteHost()).append(":").append(context.getRemotePort()) | |||
There was a problem hiding this comment.
Another thing we should consider is that when we clear the threadlocal's value.
Should we keep them in memory all the time?
There was a problem hiding this comment.
@carryxyh
If the current thread which execute this invoke is from a thread pool then I think we should keep it because then it will be reusable and will avoid the time creating new date formatter object, otherwise we should remove it.
What would you suggest to me?
There was a problem hiding this comment.
@carryxyh would you suggest is there any things I needs to change here?
Fixing some typos.
|
If we can move all date time format into org.apache.dubbo.rpc.filter.AccessLogFilter.LogTask, then we can safely make the formatter static, since it is now running on a single thread. |
I am planning for move FILE_DATE_FORMATTER to LogTask level (as you are suggesting, i also beleive it will make sense) as this is created once for each log thread(which is currently single threaded). as of now to simply intiale SimpleDateFormat once per LogTask thread instance as it is created (as of now it is not served from thread pool, when we have thread pool can change it accordingly) and MESSAGE_DATE_FORMATTER to RpcContext level. Would you correct me if this approach has problem. Any guidance will be appreciated. |
|
Hi all, would you care to review it. |
|
@beiwei30, would you suggest here me any thing ? |
|
@khanimteyaz I think we can use the newSingleThreadScheduledExecutor to create a timed task. This way we can just create a SimpleDateFormat once (bind to a LogTask). |
It make sense writing to log through this thread, but just to confirm you mean writing to log only, not the log message creation part? Performing all write through single instance by **newSingleThreadScheduledExecutor ** is good approach and I am with you on this, only thing which is stoping me how in the case of writing pending log message in case of jvm shutdown. I think jdk provide a way for this and using the scheduleWithFixedDelay of ScheduledExecutorService. @carryxyh |
|
@khanimteyaz
What I may understand about this sentence is not very accurate. What you mean is, how do you write the pending information to a file at the end of the JVM? I think we can let the scheduled thread daemon.
I think we should change the |
I am think I was wrong on the pending log message, thanks for correcting me and helping out here. Let the schedule thread to finish the pending task 😄 Regarding
I will make the changes accordingly. |
…hread and for default logger also
|
@carryxyh I have made the changes but builds failing due to travis log limit. |
|
Hi, seems like your pr has many commits of other persons. About the ci, I will have a look. |
Sure. on it. |
|
I will create a new branch where, would raised all the changes with all recommendation |
|
@carryxyh created another PR for the same #3080 As I am not sure how to reduce number of commit from existing PR, so created a new one. Sorry for the inconvenience. |
What is the purpose of the change
AcceLogFilter message creation and last and current check was performing by each time simple date formate object creation. In this version of code I have use the ThreadLocal for each thread wise to create simple date format object instead of each call
#3026
Brief changelog
XXXXX
Verifying this change
Runnig UT and verifying generated access log.
Follow this checklist to help us incorporate your contribution quickly and easily:
[Dubbo-XXX] Fix UnknownException when host config not exist #XXX. Each commit in the pull request should have a meaningful subject line and body.mvn clean install -DskipTests=false&mvn clean test-compile failsafe:integration-testto make sure unit-test and integration-test pass.