You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "khanimteyaz (GitHub)" <gi...@apache.org> on 2018/12/20 11:45:24 UTC
[GitHub] [incubator-dubbo] khanimteyaz opened pull request #3027:
Simple date format each time new object creation removed
## 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
https://github.com/apache/incubator-dubbo/issues/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:
- [x] Make sure there is a [GITHUB_issue] (https://github.com/apache/incubator-dubbo/issues/3026) field for the change (usually before you start working on it). Trivial changes like typos do not require a GITHUB issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
- [ ] Format the pull request title like `[Dubbo-XXX] Fix UnknownException when host config not exist #XXX`. Each commit in the pull request should have a meaningful subject line and body.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in [test module](https://github.com/apache/incubator-dubbo/tree/master/dubbo-test).
- [ ] Run `mvn clean install -DskipTests=false` & `mvn clean test-compile failsafe:integration-test` to make sure unit-test and integration-test pass.
- [ ] If this contribution is large, please follow the [Software Donation Guide](https://github.com/apache/incubator-dubbo/wiki/Software-donation-guide).
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3027: Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
@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?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3027: Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
Thanks for suggesting this. I was looking into RpcContext code for LOCAL and SERVER implementation, I would like to go this approach.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3027: Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
@carryxyh would you suggest is there any things I needs to change here?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on pull request #3027:
Simple date format each time new object creation removed
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
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.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on issue #3027: Simple
date format each time new object creation removed
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
> 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.
@beiwei30
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.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz closed pull request #3027:
Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
[ pull request closed by khanimteyaz ]
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3027:
Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
> 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.
@beiwei30
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.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on issue #3027: Simple
date format each time new object creation removed
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
@khanimteyaz
Hi,
I have not found a LogTask in AccessLogFilter before. After I looked at the code, I personally thought that it would be fine to put all the log output into the LogTask.
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).
How do u think?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3027:
Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
> @khanimteyaz
> Hi,
> I have not found a LogTask in AccessLogFilter before. After I looked at the code, I personally thought that it would be fine to put all the log output into the LogTask.
>
> 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).
> How do u think?
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
Do you feel, the approach I am taking is right or would you suggest me something else here on the implementation?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3027:
Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
I will create a new branch where, would raised all the changes with all recommendation
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #3027:
Simple date format each time new object creation removed
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=h1) Report
> Merging [#3027](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/1a068740a001da0a658c7839f107ae4aa03679e3?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `60%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=tree)
```diff
@@ 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
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...annotation/ServiceAnnotationBeanPostProcessor.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvYmVhbnMvZmFjdG9yeS9hbm5vdGF0aW9uL1NlcnZpY2VBbm5vdGF0aW9uQmVhblBvc3RQcm9jZXNzb3IuamF2YQ==) | `86.89% <ø> (ø)` | :arrow_up: |
| [...ing/transport/netty4/NettyBackedChannelBuffer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHk0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvbmV0dHk0L05ldHR5QmFja2VkQ2hhbm5lbEJ1ZmZlci5qYXZh) | `19.04% <ø> (ø)` | :arrow_up: |
| [...ava/org/apache/dubbo/common/utils/ConfigUtils.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vdXRpbHMvQ29uZmlnVXRpbHMuamF2YQ==) | `77.93% <ø> (ø)` | :arrow_up: |
| [...apache/dubbo/rpc/filter/tps/DefaultTPSLimiter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9maWx0ZXIvdHBzL0RlZmF1bHRUUFNMaW1pdGVyLmphdmE=) | `66.66% <ø> (ø)` | :arrow_up: |
| [...notation/ReferenceAnnotationBeanPostProcessor.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvYmVhbnMvZmFjdG9yeS9hbm5vdGF0aW9uL1JlZmVyZW5jZUFubm90YXRpb25CZWFuUG9zdFByb2Nlc3Nvci5qYXZh) | `76.38% <ø> (ø)` | :arrow_up: |
| [...ting/transport/netty/NettyBackedChannelBuffer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eUJhY2tlZENoYW5uZWxCdWZmZXIuamF2YQ==) | `0% <ø> (ø)` | :arrow_up: |
| [...he/dubbo/common/io/UnsafeByteArrayInputStream.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vaW8vVW5zYWZlQnl0ZUFycmF5SW5wdXRTdHJlYW0uamF2YQ==) | `82.92% <ø> (ø)` | :arrow_up: |
| [.../CompatibleServiceAnnotationBeanPostProcessor.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tY29tcGF0aWJsZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vY29uZmlnL3NwcmluZy9iZWFucy9mYWN0b3J5L2Fubm90YXRpb24vQ29tcGF0aWJsZVNlcnZpY2VBbm5vdGF0aW9uQmVhblBvc3RQcm9jZXNzb3IuamF2YQ==) | `0% <ø> (ø)` | :arrow_up: |
| [...pc/cluster/support/wrapper/MockClusterInvoker.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvc3VwcG9ydC93cmFwcGVyL01vY2tDbHVzdGVySW52b2tlci5qYXZh) | `71.42% <ø> (ø)` | :arrow_up: |
| [...a/org/apache/dubbo/remoting/exchange/Response.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9SZXNwb25zZS5qYXZh) | `94.73% <ø> (ø)` | :arrow_up: |
| ... and [27 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=footer). Last update [1a06874...89e5485](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on issue #3027: Simple
date format each time new object creation removed
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
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.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3027: Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
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?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3027:
Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
Hi all, would you care to review it.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on pull request #3027:
Simple date format each time new object creation removed
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
Another thing we should consider is that when we clear the threadlocal's value.
Should we keep them in memory all the time?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on issue #3027: Simple
date format each time new object creation removed
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
> 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.
@beiwei30
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.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3027:
Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
@beiwei30, would you suggest here me any thing ?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on issue #3027: Simple
date format each time new object creation removed
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
Hi, seems like your pr has many commits of other persons.
Would u pls clear your pr first? I saw that this pr has changed about 600 lines code.
About the ci, I will have a look.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #3027:
Simple date format each time new object creation removed
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=h1) Report
> Merging [#3027](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/1a068740a001da0a658c7839f107ae4aa03679e3?src=pr&el=desc) will **increase** coverage by `0.04%`.
> The diff coverage is `57.14%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3027 +/- ##
==========================================
+ Coverage 64.24% 64.29% +0.04%
==========================================
Files 584 584
Lines 26056 26060 +4
Branches 4562 4562
==========================================
+ Hits 16740 16754 +14
+ Misses 7132 7125 -7
+ Partials 2184 2181 -3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...a/org/apache/dubbo/rpc/filter/AccessLogFilter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9maWx0ZXIvQWNjZXNzTG9nRmlsdGVyLmphdmE=) | `71.27% <57.14%> (+0.16%)` | :arrow_up: |
| [...ache/dubbo/remoting/p2p/support/AbstractGroup.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctcDJwL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9wMnAvc3VwcG9ydC9BYnN0cmFjdEdyb3VwLmphdmE=) | `45.45% <0%> (-11.37%)` | :arrow_down: |
| [...onfig/spring/extension/SpringExtensionFactory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvZXh0ZW5zaW9uL1NwcmluZ0V4dGVuc2lvbkZhY3RvcnkuamF2YQ==) | `78.04% <0%> (-7.32%)` | :arrow_down: |
| [...he/dubbo/registry/multicast/MulticastRegistry.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktbXVsdGljYXN0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9tdWx0aWNhc3QvTXVsdGljYXN0UmVnaXN0cnkuamF2YQ==) | `63.79% <0%> (-1.73%)` | :arrow_down: |
| [...apache/dubbo/rpc/protocol/dubbo/DubboProtocol.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0R1YmJvUHJvdG9jb2wuamF2YQ==) | `66.66% <0%> (+0.83%)` | :arrow_up: |
| [.../exchange/support/header/HeaderExchangeServer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9IZWFkZXJFeGNoYW5nZVNlcnZlci5qYXZh) | `60.68% <0%> (+0.85%)` | :arrow_up: |
| [...rg/apache/dubbo/common/timer/HashedWheelTimer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vdGltZXIvSGFzaGVkV2hlZWxUaW1lci5qYXZh) | `62.36% <0%> (+1.74%)` | :arrow_up: |
| [...dubbo/remoting/exchange/support/DefaultFuture.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L0RlZmF1bHRGdXR1cmUuamF2YQ==) | `69.79% <0%> (+2.01%)` | :arrow_up: |
| [...dubbo/rpc/protocol/dubbo/CallbackServiceCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0NhbGxiYWNrU2VydmljZUNvZGVjLmphdmE=) | `79.41% <0%> (+2.2%)` | :arrow_up: |
| [...e/dubbo/remoting/transport/netty/NettyChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eUNoYW5uZWwuamF2YQ==) | `56.47% <0%> (+2.35%)` | :arrow_up: |
| ... and [3 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3027/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=footer). Last update [1a06874...7956c15](https://codecov.io/gh/apache/incubator-dubbo/pull/3027?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3027:
Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
> @khanimteyaz
> Hi,
> I have not found a LogTask in AccessLogFilter before. After I looked at the code, I personally thought that it would be fine to put all the log output into the 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?
>
> 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).
> How do u think?
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
Do you feel, the approach I am taking is right or would you suggest me something else here on the implementation?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3027:
Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
> Hi, seems like your pr has many commits of other persons.
> Would u pls clear your pr first? I saw that this pr has changed about 600 lines code.
>
> About the ci, I will have a look.
Sure. on it.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3027:
Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
@carryxyh I have made the changes but builds failing due to travis log limit.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3027:
Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
@carryxyh created another PR for the same https://github.com/apache/incubator-dubbo/pull/3080
As I am not sure how to reduce number of commit from existing PR, so created a new one. Sorry for the inconvenience.
Would request you to guide on this, how to reduce or clear other's commit from existing one, so that next time I can avoid creating separate PR.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3027:
Simple date format each time new object creation removed
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
@carryxyh
> 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 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 think we should change the logQueue to let us create log message in another thread.
Maybe like this: ConcurrentMap<String, Set<LogData>> logQueue. And then, our scheduled thread just check the queue regularly. This is my thought. After that, we can promise our log is print in only one thread and make the SimpleDateFormat single instance.
I will make the changes accordingly.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on issue #3027: Simple
date format each time new object creation removed
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
@khanimteyaz
IMO, the log message will create and log in schedule thread.
> how in the case of writing pending log message in case of jvm shutdown
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.
> Do you feel, the approach I am taking is right or would you suggest me something else here on the implementation?
I think we should change the `logQueue` to let us create log message in another thread.
Maybe like this: `ConcurrentMap<String, Set<LogData>> logQueue`. And then, our scheduled thread just check the queue regularly. This is my thought. After that, we can promise our log is print in only one thread and make the SimpleDateFormat single instance.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3027 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org