You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@dubbo.apache.org by "carryxyh (GitHub)" <gi...@apache.org> on 2019/01/09 03:06:12 UTC

[GitHub] [incubator-dubbo] carryxyh opened pull request #3174: Fix timeout filter not work in async way.

Fix timeout filter not work in async way.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on issue #3174: Fix timeout filter not work in async way.

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

My only concern with this PR is the accident transfer of TIMEOUT_FILTER_START_TIME along the RPC chain.  Though the real root is a drawback of the RpcContext.
Since TimeoutFilter will be included in every RPC call, it's a big thing and sth should be careful with.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "carryxyh (GitHub)" <gi...@apache.org>.

Seems like you are right...
Great suggestion, I will fix it.
👍 

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on issue #3174: Fix timeout filter not work in async way.

Posted by "carryxyh (GitHub)" <gi...@apache.org>.

I changed a way, it seems that our Invocation is unchanged, maybe we can solve our problem with a global map.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] codecov-io commented on issue #3174: Fix timeout filter not work in async way.

Posted by "codecov-io (GitHub)" <gi...@apache.org>.

# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?src=pr&el=h1) Report
> Merging [#3174](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/460c3a17568ff31f994646558a48c2d0e7b37bd4?src=pr&el=desc) will **increase** coverage by `0.07%`.
> The diff coverage is `42.85%`.

[![Impacted file tree graph](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/graphs/tree.svg?width=650&token=VnEIkiFQT0&height=150&src=pr)](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master    #3174      +/-   ##
============================================
+ Coverage     63.55%   63.63%   +0.07%     
  Complexity       75       75              
============================================
  Files           652      652              
  Lines         28200    28205       +5     
  Branches       4781     4782       +1     
============================================
+ Hits          17922    17947      +25     
+ Misses         8023     8002      -21     
- Partials       2255     2256       +1
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ava/org/apache/dubbo/rpc/filter/TimeoutFilter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9maWx0ZXIvVGltZW91dEZpbHRlci5qYXZh) | `52.94% <42.85%> (-30.4%)` | `0 <0> (ø)` | |
| [.../java/org/apache/dubbo/config/ReferenceConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9SZWZlcmVuY2VDb25maWcuamF2YQ==) | `59.24% <0%> (+0.75%)` | `0% <0%> (ø)` | :arrow_down: |
| [...dubbo/remoting/exchange/support/DefaultFuture.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L0RlZmF1bHRGdXR1cmUuamF2YQ==) | `74.49% <0%> (+1.34%)` | `0% <0%> (ø)` | :arrow_down: |
| [...rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL3RlbG5ldC9JbnZva2VUZWxuZXRIYW5kbGVyLmphdmE=) | `73.8% <0%> (+1.58%)` | `0% <0%> (ø)` | :arrow_down: |
| [...e/dubbo/remoting/transport/netty/NettyChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eUNoYW5uZWwuamF2YQ==) | `57.64% <0%> (+3.52%)` | `0% <0%> (ø)` | :arrow_down: |
| [...che/dubbo/remoting/transport/mina/MinaChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbWluYS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcmVtb3RpbmcvdHJhbnNwb3J0L21pbmEvTWluYUNoYW5uZWwuamF2YQ==) | `53.94% <0%> (+10.52%)` | `0% <0%> (ø)` | :arrow_down: |
| [.../remoting/transport/netty4/NettyClientHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHk0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvbmV0dHk0L05ldHR5Q2xpZW50SGFuZGxlci5qYXZh) | `86.11% <0%> (+11.11%)` | `0% <0%> (ø)` | :arrow_down: |
| [...ache/dubbo/remoting/p2p/support/AbstractGroup.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctcDJwL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9wMnAvc3VwcG9ydC9BYnN0cmFjdEdyb3VwLmphdmE=) | `56.81% <0%> (+11.36%)` | `0% <0%> (ø)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?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/3174?src=pr&el=footer). Last update [460c3a1...abe5600](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?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/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "carryxyh (GitHub)" <gi...@apache.org>.

It is indeed a problem. I personally think that my revision is rather stubborn. But haven't found a better way to pass the start time. . .

Maybe using a DataStore or sth else?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "carryxyh (GitHub)" <gi...@apache.org>.

Hi, @khanimteyaz 
After check the code, seems like it would throw any exception..
So I keep this code.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] codecov-io commented on issue #3174: Fix timeout filter not work in async way.

Posted by "codecov-io (GitHub)" <gi...@apache.org>.

# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?src=pr&el=h1) Report
> Merging [#3174](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/460c3a17568ff31f994646558a48c2d0e7b37bd4?src=pr&el=desc) will **increase** coverage by `0.19%`.
> The diff coverage is `36.84%`.

[![Impacted file tree graph](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/graphs/tree.svg?width=650&token=VnEIkiFQT0&height=150&src=pr)](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master    #3174      +/-   ##
============================================
+ Coverage     63.63%   63.82%   +0.19%     
  Complexity       75       75              
============================================
  Files           652      661       +9     
  Lines         28200    28633     +433     
  Branches       4781     4828      +47     
============================================
+ Hits          17944    18274     +330     
- Misses         8005     8144     +139     
+ Partials       2251     2215      -36
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ava/org/apache/dubbo/rpc/filter/TimeoutFilter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9maWx0ZXIvVGltZW91dEZpbHRlci5qYXZh) | `42.85% <36.84%> (-40.48%)` | `0 <0> (ø)` | |
| [...he/dubbo/rpc/protocol/rmi/RmiRemoteInvocation.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1ybWkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9wcm90b2NvbC9ybWkvUm1pUmVtb3RlSW52b2NhdGlvbi5qYXZh) | `0% <0%> (-100%)` | `0% <0%> (ø)` | |
| [.../spring/schema/AnnotationBeanDefinitionParser.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvc2NoZW1hL0Fubm90YXRpb25CZWFuRGVmaW5pdGlvblBhcnNlci5qYXZh) | `9.09% <0%> (-81.82%)` | `0% <0%> (ø)` | |
| [...main/java/org/apache/dubbo/rpc/cluster/Router.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvUm91dGVyLmphdmE=) | `40% <0%> (-60%)` | `0% <0%> (ø)` | |
| [...ache/dubbo/config/spring/util/AnnotationUtils.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvdXRpbC9Bbm5vdGF0aW9uVXRpbHMuamF2YQ==) | `36.98% <0%> (-40.8%)` | `0% <0%> (ø)` | |
| [...va/org/apache/dubbo/config/ConfigCenterConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9Db25maWdDZW50ZXJDb25maWcuamF2YQ==) | `13.43% <0%> (-17.61%)` | `0% <0%> (ø)` | |
| [.../org/apache/dubbo/config/spring/ReferenceBean.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvUmVmZXJlbmNlQmVhbi5qYXZh) | `19.04% <0%> (-16.81%)` | `0% <0%> (ø)` | |
| [...apache/dubbo/common/config/ConfigurationUtils.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vY29uZmlnL0NvbmZpZ3VyYXRpb25VdGlscy5qYXZh) | `40% <0%> (-16.25%)` | `0% <0%> (ø)` | |
| [...beans/factory/annotation/ReferenceBeanBuilder.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvYmVhbnMvZmFjdG9yeS9hbm5vdGF0aW9uL1JlZmVyZW5jZUJlYW5CdWlsZGVyLmphdmE=) | `71.11% <0%> (-16.07%)` | `0% <0%> (ø)` | |
| [...ava/org/apache/dubbo/config/DubboShutdownHook.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9EdWJib1NodXRkb3duSG9vay5qYXZh) | `75% <0%> (-9.38%)` | `0% <0%> (ø)` | |
| ... and [157 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?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/3174?src=pr&el=footer). Last update [460c3a1...a19cb2a](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?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/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "carryxyh (GitHub)" <gi...@apache.org>.

I know.
I have read an article and it seems that it is better to start another thread to get time regularly? But I think this is not a bottleneck right now.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "carryxyh (GitHub)" <gi...@apache.org>.

I have fix this place.
Seems like only RpcInvocation can set attachments.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on issue #3174: Fix timeout filter not work in async way.

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

I just tested locally, TimeoutFilter is behind ContextFilter and these two filters will only be triggered on the provider side, so any changes to Invocation in TimeoutFilter will not have the chance to affect RpcContext. 
My concern above is not going to happen.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on issue #3174: Fix timeout filter not work in async way.

Posted by "carryxyh (GitHub)" <gi...@apache.org>.

Nope.
That is all. @khanimteyaz  Thx for your review. 👍 

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

If there is no attachement for invocation, to handle fix timeout should we care to create a attachment and set TIMEOUT_FILTER_START_TIME ? What do you say?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

I think it's already of no importance of caring about removing the attachment or not in here.

The Invocation is going to the end of its lifecycle because RPC call has finished.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

@carryxyh 
I was thinking to have the responsibility of assigning a default TIMEOUT_FILTER_START_TIME  in RpcInvocation, but then realized that, I am going in wrong direction and it will landed up mixing  RpcInvocation Responsibility with TimerFilter. 

Just thinking loud here (please bear with me 😄 ), should we create a FilterWrapper and use it to store framework code related internal properties here, it may give us better way of separating object information. e.g

```
Class FilterWrapper {
  private Invocation target;
  private Map<String,Object> properties = new HashMap<>();
   FilterWrapper (Invocation invocation)  {
      this.target = invocation;
  }
  public void addProperties(String key,Object value) {
   properties.put(key,value);
} 
   public Map<String,Object> getProperties() {
     return this.properties;
 }
   public Map<String,Object> getAttachment() {
      this.target.getAttachment();
  }
  ..........
_more getter and setter or deleter as per needs_
  ....
}
wrapper=FilterWrapper.of(invocation); 
wrapper.addProperties(TIMEOUT_FILTER_START_TIME, String.valueOf(start));
```

So even in future we need to keep add internal properties we may not be landed up overriding user propvided values.
What do you say?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

Now I think the previous way you give is better, please see my comments below. 
So what do you think now? Would you please revert the code?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

I am not sure should we care about the scenario here, where in case of exception while executing _invoker.invoke(invocation)_ , should we remove TIMEOUT_FILTER_START_TIME from attacchment ? I think as this piece of code should take of removing TIMEOUT_FILTER_START_TIME even in case of exception. What do you say?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

I was think to have the responsibility of assigning a default TIMEOUT_FILTER_START_TIME  in RpcInvocation, but we will be landed up mixing its Responsibility with TimerFilter.  

I am just thinking loud here (please bear with me 😄 ), should we create a FilterWrapper and can store these information here, it may give us better way of separating object information. e.g

```
Class FilterWrapper {
  private Invocation target;
  private Map<String,Object> properties = new HashMap<>();
   FilterWrapper (Invocation invocation)  {
      this.target = invocation;
  }
  public void addProperties(String key,Object value) {
   properties.put(key,value);
} 
   public Map<String,Object> getProperties() {
     return this.properties;
 }
   public Map<String,Object> getAttachment() {
      this.target.getAttachment();
  }
  ..........
_more getter and setter or deleter as per needs_
  ....
}
wrapper=FilterWrapper.of(invocation); 
wrapper.addProperties(TIMEOUT_FILTER_START_TIME, String.valueOf(start));
```

So even in future we need to keep add internal properties we may not be landed up overriding user propvided values.
What do you say?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3174: Fix timeout filter not work in async way.

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

@carryxyh in this PR are planning to address anything more?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "carryxyh (GitHub)" <gi...@apache.org>.

I have checked the code.
As you said, the timeoutfilter does not affect the RpcContext, nor does it affect the invocation downstream of the call chain. I have reverted the code.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

If there are bug we any how need to fix it :). 

I think it is fine, as I was worring about whether we are missing to remove it or not, but on our known path we are removing.

I have created a issue to write a UT for specific this scenario and will take it 
https://github.com/apache/incubator-dubbo/issues/3234


[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

Be careful that this attachment `TIMEOUT_FILTER_START_TIME ` will be passed throughout the RPC chain because of the drawback of RpcContext. #3017 

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

Let's keep it easy, functionality then performance.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

@khanimteyaz That's interesting, but if we add an extra `properties` just like you did in FilterWrapper, we need to further figure out how to pass the `properties` onto the wire. But at present `invocation` is the only carrier, so everything needs to finally go into `invocation` to get transferred.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] codecov-io commented on issue #3174: Fix timeout filter not work in async way.

Posted by "codecov-io (GitHub)" <gi...@apache.org>.

# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?src=pr&el=h1) Report
> Merging [#3174](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/460c3a17568ff31f994646558a48c2d0e7b37bd4?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `40%`.

[![Impacted file tree graph](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/graphs/tree.svg?width=650&token=VnEIkiFQT0&height=150&src=pr)](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master    #3174      +/-   ##
============================================
+ Coverage     63.63%   63.63%   +<.01%     
  Complexity       75       75              
============================================
  Files           652      652              
  Lines         28200    28399     +199     
  Branches       4781     4907     +126     
============================================
+ Hits          17944    18072     +128     
- Misses         8005     8073      +68     
- Partials       2251     2254       +3
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ava/org/apache/dubbo/rpc/filter/TimeoutFilter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9maWx0ZXIvVGltZW91dEZpbHRlci5qYXZh) | `45.45% <40%> (-37.88%)` | `0 <0> (ø)` | |
| [...bo/rpc/cluster/support/FailbackClusterInvoker.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tY2x1c3Rlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL2NsdXN0ZXIvc3VwcG9ydC9GYWlsYmFja0NsdXN0ZXJJbnZva2VyLmphdmE=) | `67.21% <0%> (-8.2%)` | `0% <0%> (ø)` | |
| [...ava/org/apache/dubbo/config/DubboShutdownHook.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9EdWJib1NodXRkb3duSG9vay5qYXZh) | `78.12% <0%> (-6.25%)` | `0% <0%> (ø)` | |
| [...ng/transport/dispatcher/all/AllChannelHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvZGlzcGF0Y2hlci9hbGwvQWxsQ2hhbm5lbEhhbmRsZXIuamF2YQ==) | `51.42% <0%> (-5.72%)` | `0% <0%> (ø)` | |
| [.../org/apache/dubbo/remoting/ExecutionException.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9FeGVjdXRpb25FeGNlcHRpb24uamF2YQ==) | `15.78% <0%> (-5.27%)` | `0% <0%> (ø)` | |
| [...bbo/registry/support/ProviderConsumerRegTable.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9zdXBwb3J0L1Byb3ZpZGVyQ29uc3VtZXJSZWdUYWJsZS5qYXZh) | `80.48% <0%> (-4.88%)` | `0% <0%> (ø)` | |
| [...onfig/spring/extension/SpringExtensionFactory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvZXh0ZW5zaW9uL1NwcmluZ0V4dGVuc2lvbkZhY3RvcnkuamF2YQ==) | `80.48% <0%> (-4.88%)` | `0% <0%> (ø)` | |
| [...e/dubbo/remoting/transport/netty/NettyChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eUNoYW5uZWwuamF2YQ==) | `57.64% <0%> (-4.71%)` | `0% <0%> (ø)` | |
| [.../apache/dubbo/remoting/transport/AbstractPeer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RQZWVyLmphdmE=) | `63.04% <0%> (-4.35%)` | `0% <0%> (ø)` | |
| [...apache/dubbo/remoting/transport/AbstractCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RDb2RlYy5qYXZh) | `76.92% <0%> (-3.85%)` | `0% <0%> (ø)` | |
| ... and [24 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3174/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?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/3174?src=pr&el=footer). Last update [460c3a1...c8c1f7b](https://codecov.io/gh/apache/incubator-dubbo/pull/3174?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/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

I am not sure should we care about the scenario here, where in case of exception while executing _invoker.invoke(invocation)_ , should we remove TIMEOUT_FILTER_START_TIME from attacchment ? I think this piece of code should take care of removing TIMEOUT_FILTER_START_TIME even in case of exception. What do you say?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on issue #3174: Fix timeout filter not work in async way.

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

Oh, I just noticed that this filter is only executed on provider side `@Activate(group = Constants.PROVIDER)`. Maybe the problem I am worrying is not happening.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] Kiddinglife commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "Kiddinglife (GitHub)" <gi...@apache.org>.

`System.currentTimeMillis();` is not good choice for interval measurement.
http://sangsoonam.github.io/2017/03/01/do-not-use-curenttimemillis-for-time-interval.html
https://github.com/openshift/jenkins-plugin/issues/76
http://pzemtsov.github.io/2017/07/23/the-slow-currenttimemillis.html

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on issue #3174: Fix timeout filter not work in async way.

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

I just tested locally, TimeoutFilter is behind ContextFilter and these two filters will only be triggered on the provider side, so any changes to Invocation in ContextFilter will not have the chance to affect RpcContext. 
My concern above is not going to happen.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj closed pull request #3174: Fix timeout filter not work in async way.

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

[ pull request closed by chickenlj ]

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "carryxyh (GitHub)" <gi...@apache.org>.

Won't miss remove TIMEOUT_FILTER_START_TIME unless there is a bug.
But in bug case, we need fix our code but not add a try catch..I think.
What do u  think about it?
:)

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "carryxyh (GitHub)" <gi...@apache.org>.

Agree.
I will fix it.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

If there is no attachement for invocation, to handle fix timeout should we  care to create a attachment and set TIMEOUT_FILTER_START_TIME ? What do you say?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3174: Fix timeout filter not work in async way.

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.

> Hi, @khanimteyaz
> After check the code, seems like it would throw any exception..
> So I keep this code.

In this case would we be missing the chance to remove TIMEOUT_FILTER_START_TIME  from attachment? 

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3174 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org