You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "beiwei30 (GitHub)" <gi...@apache.org> on 2018/12/21 02:45:42 UTC
[GitHub] [incubator-dubbo] beiwei30 opened pull request #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
## What is the purpose of the change
make count work again when max <= 0
## Brief changelog
dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/RpcStatus.java
dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/filter/ActiveLimitFilter.java
dubbo-rpc/dubbo-rpc-api/src/main/java/org/apache/dubbo/rpc/filter/ExecuteLimitFilter.java
## Verifying this change
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) 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/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on issue #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
more comments? @khanimteyaz
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
I see where the problem is. Pls. review the latest commit.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
first of all, `endCount` does more than limiting the traffic only, it is also responsible for running statistics, see below:
```java
private static void endCount(RpcStatus status, long elapsed, boolean succeeded) {
status.active.decrementAndGet();
status.total.incrementAndGet();
status.totalElapsed.addAndGet(elapsed);
if (status.maxElapsed.get() < elapsed) {
status.maxElapsed.set(elapsed);
}
if (succeeded) {
if (status.succeededMaxElapsed.get() < elapsed) {
status.succeededMaxElapsed.set(elapsed);
}
} else {
status.failed.incrementAndGet();
status.failedElapsed.addAndGet(elapsed);
if (status.failedMaxElapsed.get() < elapsed) {
status.failedMaxElapsed.set(elapsed);
}
}
}
```
That's the reason we need to make sure `startCount` and `endCount` execute no matter `max` limit is set or not, but at the same time, we also need to make sure the filter will block incoming request if `max` limit exceeds.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3035?src=pr&el=h1) Report
> Merging [#3035](https://codecov.io/gh/apache/incubator-dubbo/pull/3035?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/3e6ae66fde6db2edb139e59d3c5530c9196b4144?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `100%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/3035?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3035 +/- ##
==========================================
- Coverage 64.3% 64.28% -0.03%
==========================================
Files 584 584
Lines 26056 26056
Branches 4562 4562
==========================================
- Hits 16755 16749 -6
- Misses 7118 7123 +5
- Partials 2183 2184 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3035?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [.../src/main/java/org/apache/dubbo/rpc/RpcStatus.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9ScGNTdGF0dXMuamF2YQ==) | `70% <100%> (ø)` | :arrow_up: |
| [...rg/apache/dubbo/rpc/filter/ExecuteLimitFilter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9maWx0ZXIvRXhlY3V0ZUxpbWl0RmlsdGVyLmphdmE=) | `86.66% <100%> (ø)` | :arrow_up: |
| [...org/apache/dubbo/rpc/filter/ActiveLimitFilter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9maWx0ZXIvQWN0aXZlTGltaXRGaWx0ZXIuamF2YQ==) | `97.05% <100%> (ø)` | :arrow_up: |
| [.../apache/dubbo/qos/protocol/QosProtocolWrapper.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcGx1Z2luL2R1YmJvLXFvcy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcW9zL3Byb3RvY29sL1Fvc1Byb3RvY29sV3JhcHBlci5qYXZh) | `64.1% <0%> (-17.95%)` | :arrow_down: |
| [...e/dubbo/remoting/transport/netty/NettyChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eUNoYW5uZWwuamF2YQ==) | `57.64% <0%> (-4.71%)` | :arrow_down: |
| [...c/main/java/org/apache/dubbo/rpc/RpcException.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9ScGNFeGNlcHRpb24uamF2YQ==) | `85.71% <0%> (-3.58%)` | :arrow_down: |
| [...dubbo/rpc/protocol/dubbo/CallbackServiceCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0NhbGxiYWNrU2VydmljZUNvZGVjLmphdmE=) | `77.2% <0%> (-2.21%)` | :arrow_down: |
| [...dubbo/remoting/exchange/support/DefaultFuture.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L0RlZmF1bHRGdXR1cmUuamF2YQ==) | `67.78% <0%> (-2.02%)` | :arrow_down: |
| [...apache/dubbo/rpc/protocol/dubbo/DubboProtocol.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0R1YmJvUHJvdG9jb2wuamF2YQ==) | `65.83% <0%> (-0.84%)` | :arrow_down: |
| [...pache/dubbo/registry/support/AbstractRegistry.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9zdXBwb3J0L0Fic3RyYWN0UmVnaXN0cnkuamF2YQ==) | `80.07% <0%> (-0.76%)` | :arrow_down: |
| ... and [4 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3035?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/3035?src=pr&el=footer). Last update [3e6ae66...6043fe0](https://codecov.io/gh/apache/incubator-dubbo/pull/3035?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/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3035: [DUBBO-3023]: problem in ActiveLimitFilter
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
If we add max>0 in the condition there it will cause the issue because in finally block we always reducing the endCount irrespective of max value.
e.g.
1)max value is 0 so in RpcStatus.java's begin count we will not increment the counter but in ActiveLimitFilter's invoke's finally block we will be always reducing so the final value for single invocation will landed up -1 active count.
I think not including max>0 and adding
} finally {
if (max > 0) {
RpcStatus.endCount(url, methodName, System.currentTimeMillis() - begin, isSuccess);
synchronized (count) {
count.notifyAll();
}
}
}
In ActiveLimitFilter should also work, but I beleive you can rewrite this into more clear way then we suggest.
I think we might have another issue too (not because of your changes), in a scenario if max>0 (in RpcStatus.java) get satisfied (or lets max>0 condition is not even present in code as earlier version) and the `methodStatus.active.incrementAndGet() > max` return `false` then code of else block will be executing and again it will increment the count. So for one call we are eventually incrementing counter twice. Do you feel this might be also a another problem in future?Correct me if my understanding not correct.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
can I consider this topic is resolved?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
Would it possible to add UT for the same, or would you like me to add once you done?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
Would it possible to add UT for the same, or would like me to add once you done?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3035?src=pr&el=h1) Report
> Merging [#3035](https://codecov.io/gh/apache/incubator-dubbo/pull/3035?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/3e6ae66fde6db2edb139e59d3c5530c9196b4144?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/3035?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3035 +/- ##
==========================================
+ Coverage 64.3% 64.32% +0.01%
==========================================
Files 584 584
Lines 26056 26057 +1
Branches 4562 4562
==========================================
+ Hits 16755 16760 +5
+ Misses 7118 7109 -9
- Partials 2183 2188 +5
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3035?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [.../src/main/java/org/apache/dubbo/rpc/RpcStatus.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9ScGNTdGF0dXMuamF2YQ==) | `70.32% <100%> (+0.32%)` | :arrow_up: |
| [...rg/apache/dubbo/rpc/filter/ExecuteLimitFilter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9maWx0ZXIvRXhlY3V0ZUxpbWl0RmlsdGVyLmphdmE=) | `86.66% <100%> (ø)` | :arrow_up: |
| [...org/apache/dubbo/rpc/filter/ActiveLimitFilter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9maWx0ZXIvQWN0aXZlTGltaXRGaWx0ZXIuamF2YQ==) | `97.05% <100%> (ø)` | :arrow_up: |
| [.../apache/dubbo/qos/protocol/QosProtocolWrapper.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcGx1Z2luL2R1YmJvLXFvcy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcW9zL3Byb3RvY29sL1Fvc1Byb3RvY29sV3JhcHBlci5qYXZh) | `69.23% <0%> (-12.83%)` | :arrow_down: |
| [.../apache/dubbo/remoting/transport/AbstractPeer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RQZWVyLmphdmE=) | `58.69% <0%> (-4.35%)` | :arrow_down: |
| [...c/main/java/org/apache/dubbo/rpc/RpcException.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9ScGNFeGNlcHRpb24uamF2YQ==) | `85.71% <0%> (-3.58%)` | :arrow_down: |
| [...e/dubbo/remoting/transport/netty/NettyChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eUNoYW5uZWwuamF2YQ==) | `61.17% <0%> (-1.18%)` | :arrow_down: |
| [...rg/apache/dubbo/common/timer/HashedWheelTimer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vdGltZXIvSGFzaGVkV2hlZWxUaW1lci5qYXZh) | `62.02% <0%> (-0.35%)` | :arrow_down: |
| [...pache/dubbo/registry/support/AbstractRegistry.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9zdXBwb3J0L0Fic3RyYWN0UmVnaXN0cnkuamF2YQ==) | `81.2% <0%> (+0.37%)` | :arrow_up: |
| [...che/dubbo/remoting/transport/mina/MinaChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbWluYS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcmVtb3RpbmcvdHJhbnNwb3J0L21pbmEvTWluYUNoYW5uZWwuamF2YQ==) | `53.94% <0%> (+10.52%)` | :arrow_up: |
| ... and [2 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3035/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3035?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/3035?src=pr&el=footer). Last update [3e6ae66...5268722](https://codecov.io/gh/apache/incubator-dubbo/pull/3035?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/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
If we add max>0 in the condition there it will cause the issue because in finally block we always reducing the endCount irrespective of max value.
e.g.
1)max value is 0 so in RpcStatus.java's begin count we will not increment the counter but in ActiveLimitFilter's invoke's finally block we will be always reducing so the final value for single invocation will landed up -1 active count.
I think not including max>0 and adding
```java
} finally {
if (max > 0) {
RpcStatus.endCount(url, methodName, System.currentTimeMillis() - begin, isSuccess);
synchronized (count) {
count.notifyAll();
}
}
}
```
In ActiveLimitFilter should also work, but I beleive you can rewrite this into more clear way then we suggest.
I think we might have another issue too (not because of your changes), in a scenario if max>0 (in RpcStatus.java) get satisfied (or lets max>0 condition is not even present in code as earlier version) and the `methodStatus.active.incrementAndGet() > max` return `false` then code of else block will be executing and again it will increment the count. So for one call we are eventually incrementing counter twice. Do you feel this might be also a another problem in future?Correct me if my understanding not correct.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on issue #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
more comments, @khanimteyaz
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 closed pull request #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
[ pull request closed by beiwei30 ]
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
Would it possible to add UT for the same, or would you like me to add once you are done?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
In my current implementation:
```java
if (max > 0 && methodStatus.active.incrementAndGet() > max) {
return false;
} else {
return true;
}
```
it satisfies what I explained above. When limit exceeds, it will return false, and since it is atomic operation, it can make sure further incoming requests to wait. If limit doesn't exceeds or is not set, then it will return true and allow the requests to continue, instead of wait.
More important, if we decide to allow the request to proceed, we need to make sure 'startCount' and 'endCount' to be called in pair.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
> > Would it possible to add UT for the same, or would you like me to add once you are done?
>
> since you are in the middle of adding more unit test for this class, I'm prefer to leave it to you, what do you think :)
Yes fine with me. Will take care of it once it is merge.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on issue #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
> Would it possible to add UT for the same, or would you like me to add once you are done?
since you are in the middle of adding more unit test for this class, I'm prefer to leave it to you, what do you think :)
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3035:
[DUBBO-3023]: problem in ActiveLimitFilter
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
> more comments? @khanimteyaz
You have made it such clean beautiful that I am comment less 😃 . I appreciate your effort. Thanks for taking care of cases.
PR looks good to me.
As I don't have permission I can't merge it, some one have to merge it.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3035: [DUBBO-3023]: problem in ActiveLimitFilter
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
I am below UT for testing and getting assert failure because expected is 0 and actual is -1.
```
@Test()
public void testInvokeRuntimeExceptionWithActiveCountMatch() {
URL url = URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0");
Invoker<ActiveLimitFilterTest> invoker = new RuntimeExceptionInvoker(url);
Invocation invocation = new MockInvocation();
RpcStatus count = RpcStatus.getStatus(invoker.getUrl(), invocation.getMethodName());
int beforeExceptionActiveCount = count.getActive();
try {
activeLimitFilter.invoke(invoker, invocation);
} catch (RuntimeException ex) {
int afterExceptionActiveCount = count.getActive();
assertEquals("After exception active count should be same"
, beforeExceptionActiveCount, afterExceptionActiveCount);
}
}
```
Do you see any issue here?
*Note:To run this UT i have copy pasted your PR files.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3035 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org