You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "CrazyHZM (GitHub)" <gi...@apache.org> on 2018/12/21 08:48:01 UTC
[GitHub] [incubator-dubbo] CrazyHZM opened pull request #3043: wrong
event setting
## What is the purpose of the change
Event setting is wrong when the event is READONLY_EVENT.
## Brief changelog
XXXXX
## Verifying this change
XXXXX
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/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #3043: wrong
event setting
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=h1) Report
> Merging [#3043](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/ddb518de3ab3b54ec4b6a8a52db6cde1b3f58262?src=pr&el=desc) will **decrease** coverage by `0.12%`.
> The diff coverage is `80%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3043 +/- ##
============================================
- Coverage 63.69% 63.56% -0.13%
Complexity 75 75
============================================
Files 652 652
Lines 28199 28200 +1
Branches 4782 4782
============================================
- Hits 17960 17925 -35
- Misses 7988 8016 +28
- Partials 2251 2259 +8
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rg/apache/dubbo/rpc/protocol/dubbo/DubboCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0R1YmJvQ29kZWMuamF2YQ==) | `60.2% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...dubbo/remoting/exchange/support/DefaultFuture.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L0RlZmF1bHRGdXR1cmUuamF2YQ==) | `72.97% <100%> (-2.87%)` | `0 <0> (ø)` | |
| [...ng/exchange/support/header/HeartbeatTimerTask.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9IZWFydGJlYXRUaW1lclRhc2suamF2YQ==) | `75% <100%> (+1.31%)` | `0 <0> (ø)` | :arrow_down: |
| [.../exchange/support/header/HeaderExchangeServer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9IZWFkZXJFeGNoYW5nZVNlcnZlci5qYXZh) | `60.16% <100%> (+0.34%)` | `0 <0> (ø)` | :arrow_down: |
| [...e/dubbo/remoting/exchange/codec/ExchangeCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9jb2RlYy9FeGNoYW5nZUNvZGVjLmphdmE=) | `77.06% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...va/org/apache/dubbo/remoting/exchange/Request.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9SZXF1ZXN0LmphdmE=) | `83.72% <33.33%> (-2.33%)` | `0 <0> (ø)` | |
| [.../apache/dubbo/qos/protocol/QosProtocolWrapper.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcGx1Z2luL2R1YmJvLXFvcy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcW9zL3Byb3RvY29sL1Fvc1Byb3RvY29sV3JhcHBlci5qYXZh) | `64.1% <0%> (-17.95%)` | `0% <0%> (ø)` | |
| [.../remoting/transport/netty4/NettyServerHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHk0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvbmV0dHk0L05ldHR5U2VydmVySGFuZGxlci5qYXZh) | `73.52% <0%> (-11.77%)` | `0% <0%> (ø)` | |
| [...ng/transport/dispatcher/all/AllChannelHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/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/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9FeGVjdXRpb25FeGNlcHRpb24uamF2YQ==) | `15.78% <0%> (-5.27%)` | `0% <0%> (ø)` | |
| ... and [8 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?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/3043?src=pr&el=footer). Last update [ddb518d...5dd1913](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?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/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3043: wrong event setting
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
It is always preferable to have more simplified code if can make it 😄 . In this case if READONLY_EVENT value become empty which is big change because it might involve changes of user code so then we can still add the judgement back (in future we might even have to add more conditions to judgement).
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3043:
wrong event setting
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
the original `setEvent` is necessary to keep, so we will end up with two `setEvent` methods:
```java
void setEvent(bool);
void setEvent(String);
```
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #3043: wrong
event setting
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=h1) Report
> Merging [#3043](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/e095bd9bab6c9527ee4da7031f49575651af1696?src=pr&el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `100%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3043 +/- ##
===========================================
+ Coverage 63.56% 63.6% +0.03%
Complexity 75 75
===========================================
Files 652 652
Lines 28199 28200 +1
Branches 4782 4782
===========================================
+ Hits 17926 17937 +11
+ Misses 8016 8007 -9
+ Partials 2257 2256 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...va/org/apache/dubbo/remoting/exchange/Request.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9SZXF1ZXN0LmphdmE=) | `86.66% <100%> (+0.62%)` | `0 <0> (ø)` | :arrow_down: |
| [...rg/apache/dubbo/rpc/protocol/dubbo/DubboCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0R1YmJvQ29kZWMuamF2YQ==) | `60.2% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...e/dubbo/remoting/exchange/codec/ExchangeCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9jb2RlYy9FeGNoYW5nZUNvZGVjLmphdmE=) | `77.06% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...dubbo/remoting/exchange/support/DefaultFuture.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L0RlZmF1bHRGdXR1cmUuamF2YQ==) | `71.62% <100%> (-1.54%)` | `0 <0> (ø)` | |
| [.../remoting/transport/netty4/NettyClientHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHk0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvbmV0dHk0L05ldHR5Q2xpZW50SGFuZGxlci5qYXZh) | `77.77% <0%> (-8.34%)` | `0% <0%> (ø)` | |
| [...ng/transport/dispatcher/all/AllChannelHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/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/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9FeGVjdXRpb25FeGNlcHRpb24uamF2YQ==) | `15.78% <0%> (-5.27%)` | `0% <0%> (ø)` | |
| [...he/dubbo/remoting/transport/netty/NettyServer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eVNlcnZlci5qYXZh) | `67.85% <0%> (-3.58%)` | `0% <0%> (ø)` | |
| [...exchange/support/header/HeaderExchangeHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9IZWFkZXJFeGNoYW5nZUhhbmRsZXIuamF2YQ==) | `55.81% <0%> (-2.33%)` | `0% <0%> (ø)` | |
| ... and [7 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?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/3043?src=pr&el=footer). Last update [e095bd9...1905aac](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?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/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3043:
wrong event setting
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
@carryxyh @beiwei30 could you also have an look into this PR. If it looks good to you then we can merge it.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] CrazyHZM commented on pull request #3043:
wrong event setting
Posted by "CrazyHZM (GitHub)" <gi...@apache.org>.
Yes, your understanding is correct.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3043: wrong event setting
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
@CrazyHZM thanks for info.
In the scenario if the request broken then still we are proceeding with building the request object and setting flag to indicate broken request (req.setBroken(true)). Just trying to understand (thinking loud 😄 ), after setting `req.setBroken(true)` where and how we are going to leverage this info?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] CrazyHZM commented on issue #3043: wrong
event setting
Posted by "CrazyHZM (GitHub)" <gi...@apache.org>.
> @CrazyHZM I leave more comments, pls. take a look. I realize we need to keep the original `setEvent` method.
I have modified it
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] CrazyHZM commented on issue #3043: wrong
event setting
Posted by "CrazyHZM (GitHub)" <gi...@apache.org>.
@beiwei30 I have modified , can you review it , pls.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] CrazyHZM commented on pull request #3043:
wrong event setting
Posted by "CrazyHZM (GitHub)" <gi...@apache.org>.
Yes, as far as I know, dubbo has a timeout retry mechanism
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #3043: wrong
event setting
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=h1) Report
> Merging [#3043](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?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 `76.19%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3043 +/- ##
==========================================
+ Coverage 64.3% 64.32% +0.01%
==========================================
Files 584 584
Lines 26056 26069 +13
Branches 4562 4568 +6
==========================================
+ Hits 16755 16768 +13
+ Misses 7118 7114 -4
- Partials 2183 2187 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/dubbo/remoting/exchange/Request.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9SZXF1ZXN0LmphdmE=) | `86.66% <100%> (+0.62%)` | :arrow_up: |
| [...e/dubbo/remoting/exchange/codec/ExchangeCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9jb2RlYy9FeGNoYW5nZUNvZGVjLmphdmE=) | `76.78% <100%> (-0.28%)` | :arrow_down: |
| [...dubbo/remoting/exchange/support/DefaultFuture.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L0RlZmF1bHRGdXR1cmUuamF2YQ==) | `67.56% <100%> (-2.24%)` | :arrow_down: |
| [...rg/apache/dubbo/rpc/protocol/dubbo/DubboCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0R1YmJvQ29kZWMuamF2YQ==) | `55.76% <44.44%> (-4.44%)` | :arrow_down: |
| [.../apache/dubbo/qos/protocol/QosProtocolWrapper.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcGx1Z2luL2R1YmJvLXFvcy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcW9zL3Byb3RvY29sL1Fvc1Byb3RvY29sV3JhcHBlci5qYXZh) | `69.23% <0%> (-12.83%)` | :arrow_down: |
| [...onfig/spring/extension/SpringExtensionFactory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvZXh0ZW5zaW9uL1NwcmluZ0V4dGVuc2lvbkZhY3RvcnkuamF2YQ==) | `80.48% <0%> (-4.88%)` | :arrow_down: |
| [...dubbo/rpc/protocol/dubbo/CallbackServiceCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0NhbGxiYWNrU2VydmljZUNvZGVjLmphdmE=) | `77.2% <0%> (-2.21%)` | :arrow_down: |
| [...rg/apache/dubbo/common/timer/HashedWheelTimer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vdGltZXIvSGFzaGVkV2hlZWxUaW1lci5qYXZh) | `60.62% <0%> (-1.75%)` | :arrow_down: |
| [...apache/dubbo/rpc/protocol/dubbo/DubboProtocol.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0R1YmJvUHJvdG9jb2wuamF2YQ==) | `65.83% <0%> (-0.84%)` | :arrow_down: |
| ... and [7 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?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/3043?src=pr&el=footer). Last update [3e6ae66...ab7a150](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?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/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 closed pull request #3043: wrong
event setting
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
[ pull request closed by beiwei30 ]
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] CrazyHZM commented on pull request #3043:
wrong event setting
Posted by "CrazyHZM (GitHub)" <gi...@apache.org>.
Thank you,It seems that you are right now, but if the value of READONLY_EVENT becomes empty in the future, this will be a hidden danger. Of course, this does not change much from the current point of view.
What are the advantages of removing the judgment?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3043:
wrong event setting
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
looks like we should remove
```java
public void setEvent(String event) {
mEvent = true;
mData = event;
}
```
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on issue #3043: wrong
event setting
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
This change now looks good to me.
It looks like we should do the same thing for response, change
```java
if ((flag & FLAG_EVENT) != 0) {
res.setEvent(Response.HEARTBEAT_EVENT);
}
```
to
```java
if ((flag & FLAG_EVENT) != 0) {
res.setEvent(true);
}
```
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3043: wrong event setting
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
@CrazyHZM thanks this.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] CrazyHZM commented on pull request #3043:
wrong event setting
Posted by "CrazyHZM (GitHub)" <gi...@apache.org>.
It was used in handleRequest method of HeaderExchangeHandler.java
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] CrazyHZM commented on pull request #3043:
wrong event setting
Posted by "CrazyHZM (GitHub)" <gi...@apache.org>.
There are currently only two events here, and the value of READONLY_EVENT is empty by default, so when this is an event and the data is empty, this is a READONLY_EVENT.
It has no default event.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3043: wrong event setting
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
With the limited knowledege about this part (so @chickenlj @carryxyh @beiwei30 correct me if I am not correct). I think we should either have a default event type if noting is passed in data(more informative way and declaring a default behaviour) or we should reject the request if no default behaviour is spported. It will make the easy to debug and reason about the issue.
If we provide some default behaviour (event type) and which is not communicated, for user it might be confusiong (because it is not documented or communicated to them).
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #3043: wrong
event setting
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=h1) Report
> Merging [#3043](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/460c3a17568ff31f994646558a48c2d0e7b37bd4?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `100%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3043 +/- ##
============================================
- Coverage 63.63% 63.59% -0.04%
Complexity 75 75
============================================
Files 652 652
Lines 28200 28201 +1
Branches 4781 4781
============================================
- Hits 17944 17935 -9
- Misses 8005 8010 +5
- Partials 2251 2256 +5
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...va/org/apache/dubbo/remoting/exchange/Request.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9SZXF1ZXN0LmphdmE=) | `86.66% <100%> (+0.62%)` | `0 <0> (ø)` | :arrow_down: |
| [...rg/apache/dubbo/rpc/protocol/dubbo/DubboCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0R1YmJvQ29kZWMuamF2YQ==) | `60.2% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...e/dubbo/remoting/exchange/codec/ExchangeCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9jb2RlYy9FeGNoYW5nZUNvZGVjLmphdmE=) | `77.06% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...dubbo/remoting/exchange/support/DefaultFuture.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L0RlZmF1bHRGdXR1cmUuamF2YQ==) | `72.97% <100%> (-1.53%)` | `0 <0> (ø)` | |
| [.../apache/dubbo/remoting/transport/AbstractPeer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RQZWVyLmphdmE=) | `58.69% <0%> (-8.7%)` | `0% <0%> (ø)` | |
| [...ng/transport/dispatcher/all/AllChannelHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/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/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9FeGVjdXRpb25FeGNlcHRpb24uamF2YQ==) | `15.78% <0%> (-5.27%)` | `0% <0%> (ø)` | |
| [...onfig/spring/extension/SpringExtensionFactory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1zcHJpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9zcHJpbmcvZXh0ZW5zaW9uL1NwcmluZ0V4dGVuc2lvbkZhY3RvcnkuamF2YQ==) | `80.48% <0%> (-4.88%)` | `0% <0%> (ø)` | |
| [...c/main/java/org/apache/dubbo/rpc/RpcException.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9ScGNFeGNlcHRpb24uamF2YQ==) | `82.75% <0%> (-3.45%)` | `0% <0%> (ø)` | |
| [...exchange/support/header/HeaderExchangeHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9IZWFkZXJFeGNoYW5nZUhhbmRsZXIuamF2YQ==) | `55.81% <0%> (-2.33%)` | `0% <0%> (ø)` | |
| ... and [4 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?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/3043?src=pr&el=footer). Last update [460c3a1...923466a](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?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/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] CrazyHZM commented on issue #3043: wrong
event setting
Posted by "CrazyHZM (GitHub)" <gi...@apache.org>.
@beiwei30 I have modified it
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3043: wrong event setting
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
thanks
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on issue #3043: wrong
event setting
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
@CrazyHZM, would you mind to take a look at my comment? I think the root cause is
```java
if ((flag & FLAG_EVENT) != 0) {
res.setEvent(Response.HEARTBEAT_EVENT);
}
```
once we change it to
```java
if ((flag & FLAG_EVENT) != 0) {
res.setEvent(true);
}
```
then we should not need the following logic any longer:
```java
if (!req.isBroken()) {
if ((flag & FLAG_EVENT) != 0) {
if (data != null && data.equals(Request.READONLY_EVENT)) {
req.setEvent(Request.READONLY_EVENT);
} else {
req.setEvent(Request.HEARTBEAT_EVENT);
}
}
req.setData(data);
} else {
if ((flag & FLAG_EVENT) != 0) {
req.setEvent(true);
}
}
```
And, in your change, you consider request only, pls. take care response too.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3043: wrong event setting
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
`if(Request.READONLY_EVENT.equals(data)` would be more simplified i think.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #3043: wrong
event setting
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=h1) Report
> Merging [#3043](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/0e0fbd5db890a6f976d7e45e6af3516efc2aa076?src=pr&el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `100%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3043 +/- ##
============================================
+ Coverage 63.64% 63.68% +0.03%
Complexity 75 75
============================================
Files 653 653
Lines 28218 28219 +1
Branches 4784 4784
============================================
+ Hits 17960 17970 +10
+ Misses 7996 7985 -11
- Partials 2262 2264 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...va/org/apache/dubbo/remoting/exchange/Request.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9SZXF1ZXN0LmphdmE=) | `86.66% <100%> (+0.62%)` | `0 <0> (ø)` | :arrow_down: |
| [...rg/apache/dubbo/rpc/protocol/dubbo/DubboCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0R1YmJvQ29kZWMuamF2YQ==) | `60.2% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...e/dubbo/remoting/exchange/codec/ExchangeCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9jb2RlYy9FeGNoYW5nZUNvZGVjLmphdmE=) | `77.06% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...dubbo/remoting/exchange/support/DefaultFuture.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L0RlZmF1bHRGdXR1cmUuamF2YQ==) | `72.97% <100%> (+2.5%)` | `0 <0> (ø)` | :arrow_down: |
| [...bbo/registry/support/ProviderConsumerRegTable.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9zdXBwb3J0L1Byb3ZpZGVyQ29uc3VtZXJSZWdUYWJsZS5qYXZh) | `80.48% <0%> (-4.88%)` | `0% <0%> (ø)` | |
| [...e/dubbo/remoting/transport/netty/NettyChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eUNoYW5uZWwuamF2YQ==) | `57.64% <0%> (-4.71%)` | `0% <0%> (ø)` | |
| [...he/dubbo/remoting/transport/netty/NettyServer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eVNlcnZlci5qYXZh) | `67.85% <0%> (-3.58%)` | `0% <0%> (ø)` | |
| [.../java/org/apache/dubbo/config/ReferenceConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9SZWZlcmVuY2VDb25maWcuamF2YQ==) | `58.49% <0%> (-0.38%)` | `0% <0%> (ø)` | |
| [...pache/dubbo/registry/support/AbstractRegistry.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9zdXBwb3J0L0Fic3RyYWN0UmVnaXN0cnkuamF2YQ==) | `81.15% <0%> (+0.38%)` | `0% <0%> (ø)` | :arrow_down: |
| [...a/org/apache/dubbo/monitor/dubbo/DubboMonitor.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree#diff-ZHViYm8tbW9uaXRvci9kdWJiby1tb25pdG9yLWRlZmF1bHQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL21vbml0b3IvZHViYm8vRHViYm9Nb25pdG9yLmphdmE=) | `89.71% <0%> (+1.86%)` | `0% <0%> (ø)` | :arrow_down: |
| ... and [6 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3043/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?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/3043?src=pr&el=footer). Last update [0e0fbd5...67811f6](https://codecov.io/gh/apache/incubator-dubbo/pull/3043?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/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] CrazyHZM commented on issue #3043: wrong
event setting
Posted by "CrazyHZM (GitHub)" <gi...@apache.org>.
> @CrazyHZM, would you mind to take a look at my comment? I think the root cause is
>
> ```java
> if ((flag & FLAG_EVENT) != 0) {
> res.setEvent(Response.HEARTBEAT_EVENT);
> }
> ```
>
> once we change it to
>
> ```java
> if ((flag & FLAG_EVENT) != 0) {
> res.setEvent(true);
> }
> ```
>
> then we should not need the following logic any longer:
>
> ```java
> if (!req.isBroken()) {
> if ((flag & FLAG_EVENT) != 0) {
> if (data != null && data.equals(Request.READONLY_EVENT)) {
> req.setEvent(Request.READONLY_EVENT);
> } else {
> req.setEvent(Request.HEARTBEAT_EVENT);
> }
> }
> req.setData(data);
> } else {
> if ((flag & FLAG_EVENT) != 0) {
> req.setEvent(true);
> }
> }
> ```
>
> And, in your change, you consider request only, pls. take care response too.
I agree with you. If you do this, you can guarantee the scalability of the event in the future. I will modify it.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3043: wrong event setting
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
Just for my curiosity wanted to know, in case if request is broken (I am guessing if we encounter exception and at the catch block means request is broken) then still we are proceeding and bulding the request object ?Is this understanding of my is correct?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] CrazyHZM commented on pull request #3043:
wrong event setting
Posted by "CrazyHZM (GitHub)" <gi...@apache.org>.
If this is bad request，`req.setData(t);` will be executed，so I think `if (!req.isBroken())` is necessary.
About `if ((flag & FLAG_EVENT) != 0)` , I think that plus him is just for insurance .
for example value of data is "R" but it is not an event.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3043: wrong event setting
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
Thanks for explaining this. but what I was saying is for line no 192, is
instead of having `if (data != null && data.equals(Request.READONLY_EVENT))` we could simplify this by just having
`if(Request.READONLY_EVENT.equals(data))`
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3043: wrong event setting
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
Setting only the mEvent indicate that there an event but it does tell what kind of event it was. So should we set just set single value? I beleive setting only boolean may not provide complete information what do you say?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3043:
wrong event setting
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
now I see why the original setEvent() is necessary, pls. keep it. Sorry about this.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3043: wrong event setting
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
For the shake of mine understanding `if(flag & FLAG_REQUEST) == 0) ` true, then it is response scenario and if it is false means a request has been made to the service(which can be normal request, heartbeat or event received ). Is this understanding of mine correct?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] CrazyHZM commented on pull request #3043:
wrong event setting
Posted by "CrazyHZM (GitHub)" <gi...@apache.org>.
> Setting only the mEvent indicate that there an event but it does tell what kind of event it was. So should we set just set single value? I beleive setting only boolean may not provide complete information what do you say?
Because mData will carry the relevant value, so here is simply to mark whether the request is an event
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] CrazyHZM commented on issue #3043: wrong
event setting
Posted by "CrazyHZM (GitHub)" <gi...@apache.org>.
@beiwei30 could you have an look into this PR.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on issue #3043: wrong
event setting
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
@CrazyHZM I leave more comments, pls. take a look. I realize we need to keep the original `setEvent` method.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3043:
wrong event setting
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
I don't think we can remove this, since we will decode event data later. In my opinion we should do this:
```java
if ((flag & FLAG_EVENT) != 0) {
request.setEvent(true);
}
try {
if (req.isEvent()) {
data = decodeEventData(channel, in);
} else {
data = decodeRequestData(channel, in);
}
request.setData(data);
}
```
I don't think we should distinguish event type any longer
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on pull request
#3043: wrong event setting
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
In case if the condition `if ((flag & FLAG_EVENT) != 0)` does not gets satisfied then, what will be the event value of request object? Is there any default event type?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3043 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org