Direct return when the server goes down unnormally. by carryxyh · Pull Request #2185 · apache/dubbo

carryxyh · 2018-08-04T05:29:59Z

Keep the unfinished request in every channel.
When the server goes down un-normally, return the unfinished requests directly.
The current way is to wait until timeout.

issue:
#2184

diecui1202 · 2018-08-06T06:58:18Z

@carryxyh please check the travis ci failure.

carryxyh · 2018-08-06T07:00:38Z

@diecui1202
yeah. maybe still some problems. I will try to fix them later.

carryxyh · 2018-08-07T07:54:40Z

@diecui1202
Already fix unit test.

codecov-io · 2018-08-07T07:54:46Z

Codecov Report

Merging #2185 into master will increase coverage by 0.23%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #2185      +/-   ##
============================================
+ Coverage     54.32%   54.55%   +0.23%     
- Complexity     5112     5152      +40     
============================================
  Files           559      570      +11     
  Lines         24996    25057      +61     
  Branches       4456     4459       +3     
============================================
+ Hits          13578    13671      +93     
+ Misses         9386     9345      -41     
- Partials       2032     2041       +9

Impacted Files	Coverage Δ	Complexity Δ
...a/org/apache/dubbo/remoting/exchange/Response.java	`92.1% <ø> (ø)`	`18 <0> (ø)`	⬇️
...c/main/java/org/apache/dubbo/remoting/Channel.java	`0% <0%> (ø)`	`0 <0> (?)`
...dubbo/remoting/exchange/support/DefaultFuture.java	`7.09% <0%> (-0.16%)`	`2 <0> (ø)`
...pache/dubbo/remoting/transport/AbstractClient.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...exchange/support/header/HeaderExchangeHandler.java	`29.68% <0%> (-0.48%)`	`10 <0> (ø)`
...ache/dubbo/remoting/transport/AbstractChannel.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ache/dubbo/remoting/p2p/support/AbstractGroup.java	`45.45% <0%> (-11.37%)`	`7% <0%> (-1%)`
...ubbo/rpc/protocol/dubbo/ChannelWrappedInvoker.java	`41.66% <0%> (-8.34%)`	`3% <0%> (ø)`
...rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java	`51.89% <0%> (-3.8%)`	`11% <0%> (-2%)`
...bo/remoting/transport/netty/NettyCodecAdapter.java	`53.12% <0%> (-1.57%)`	`3% <0%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70e1e62...22948cc. Read the comment docs.

diecui1202 · 2018-08-07T08:09:12Z

...bo-rpc-dubbo/src/test/java/org/apache/dubbo/rpc/protocol/dubbo/DubboInvokerAvilableTest.java

    }

    @Test
+    @Ignore


Why ignore this test case ?

I didn't ignore this.
Maybe it is a outdated content ?

diecui1202 · 2018-08-07T08:19:39Z

...ubbo/src/test/java/org/apache/dubbo/rpc/protocol/dubbo/ReferenceCountExchangeClientTest.java

-import org.junit.Before;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.*;


It's not a good practice to import *

Ok, fix it soon.

import * can't pass check-style when submitting a PR

kimmking · 2018-08-07T08:22:23Z

...o-rpc/dubbo-rpc-dubbo/src/test/java/org/apache/dubbo/rpc/protocol/dubbo/MultiThreadTest.java

    private Protocol protocol = ExtensionLoader.getExtensionLoader(Protocol.class).getAdaptiveExtension();
    private ProxyFactory proxy = ExtensionLoader.getExtensionLoader(ProxyFactory.class).getAdaptiveExtension();

+    @Ignore


there is an ignore too.

Fix it soon :)

kimmking · 2018-08-07T08:23:07Z

...ubbo/src/test/java/org/apache/dubbo/rpc/protocol/dubbo/ReferenceCountExchangeClientTest.java

-import org.junit.Before;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.*;


import * can't pass check-style when submitting a PR

carryxyh · 2018-08-08T03:19:38Z

@kimmking
@diecui1202
Fix the unit test and import *.
Review again pls :)

chickenlj · 2018-08-13T08:20:00Z

I am not sure if Channel keeps the status of requests (UN_FINISH_REQUESTS_MAP) is a good practice. For one reason, Channel is equivalent for both client and server, but only the client cares about the status of requests.

Maybe keep the status of requests on DefaultFuture can be better?

zonghaishang · 2018-08-13T09:09:26Z

I agree with Li Jun’s point of view.
Server graceful shutdown processing mechanism：

1. Unregister service from registry
2. Notify the client of the readonly event in real time（readonly event）
3. If the client settings timeout is not appropriate, the server will automatically close the connection, and the client will also receive notifications or timeouts.

The current elegant downtime work is relatively good.

carryxyh · 2018-08-13T12:41:20Z

Maybe keep the status of requests on DefaultFuture can be better?
Ok, that is more clearly.
Fix soon :)

When the server goes down un-normally, return the unfinished requests directly. The current way is to wait until timeout.

beiwei30 · 2018-08-22T13:35:33Z

...bbo-remoting-api/src/main/java/org/apache/dubbo/remoting/exchange/support/DefaultFuture.java

+     *
+     * @param channel
+     */
+    public static void closeChannel(Channel channel) {


I guess we may possibly avoid introducing UNFINISHED_REQUESTS by doing this:

for (long id : CHANNELS.keySet()) { if (channel.equals(CHANNELS.get(id))) { DefaultFuture future = getFuture(id); if (!future.isDone()) { Response disconnectResponse = new Response(r.getId()); disconnectResponse.setStatus(Response.CHANNEL_INACTIVE); disconnectResponse.setErrorMessage("Channel " + channel + " is inactive. Directly return the unFinished request."); DefaultFuture.received(channel, disconnectResponse); } } }

Looks good to me! I’ll fix it soon.

* Keep the unfinished request in every channel. When the server goes down un-normally, return the unfinished requests directly. The current way is to wait until timeout. * default method * direct return unfinished requests when remote inactive or disconnect * direct return unfinished requests when remote inactive or disconnect * name and doc * name and doc * fix unit test * fix unit test * fix unit test * close heartbeat to fix unit test * sth test * sth test * sth test * sth test * close when client close * fix unit test * fix unit test * fix unit test * ignore remote invoke unit test * don't clear request when channelInactive * replace import * * remove unuse import * optimize * optimize * fix ci failed * fix ci failed * fix ci failed * fix ci failed * fix ci failed * fix ci failed * fix ci failed * optimize unfinished request keeper * rebase onto master * fix review code (cherry picked from commit c6fd684)

diecui1202 reviewed Aug 7, 2018

View reviewed changes

kimmking reviewed Aug 7, 2018

View reviewed changes

chickenlj mentioned this pull request Aug 8, 2018

[Dubbo-#2184] Return the hanging client requests immediately when connection is broken #2184 #2197

Closed

diecui1202 added the status/waiting-for-feedback Need reporters to triage label Aug 17, 2018

carryxyh added 17 commits August 21, 2018 19:04

Keep the unfinished request in every channel.

f2c59b9

When the server goes down un-normally, return the unfinished requests directly. The current way is to wait until timeout.

default method

29c4f4e

direct return unfinished requests when remote inactive or disconnect

348b8e4

direct return unfinished requests when remote inactive or disconnect

60f49ca

name and doc

51a90ae

name and doc

dd0f8c5

fix unit test

11be40d

fix unit test

076f648

fix unit test

35d3065

close heartbeat to fix unit test

9fb70d3

sth test

c049d26

sth test

467138c

sth test

b01dfa8

sth test

b32df7e

close when client close

fd04bb4

fix unit test

e39034c

fix unit test

c745802

carryxyh added 16 commits August 21, 2018 19:06

fix unit test

6bbc849

ignore remote invoke unit test

76b909f

don't clear request when channelInactive

f3f2fca

replace import *

2429402

remove unuse import

35785b7

optimize

ee8723a

optimize

36926b8

fix ci failed

89070b3

fix ci failed

41a9733

fix ci failed

aa02691

fix ci failed

45a81a3

fix ci failed

482be3d

fix ci failed

b41eb41

fix ci failed

d800e8d

optimize unfinished request keeper

b423621

rebase onto master

b8283e3

carryxyh force-pushed the server-down branch from 655b14f to b8283e3 Compare August 21, 2018 11:13

beiwei30 reviewed Aug 22, 2018

View reviewed changes

fix review code

1079239

beiwei30 merged commit c6fd684 into apache:master Aug 23, 2018

carryxyh deleted the server-down branch August 23, 2018 08:45

This was referenced Aug 29, 2018

服务调用的超时问题 Some issues with service timeout #1968

Closed

Direct return when the server goes down unnormally. #2451

Merged

                   }
                   @Test
+                  @Ignore

Conversation

carryxyh commented Aug 4, 2018

Uh oh!

diecui1202 commented Aug 6, 2018

Uh oh!

carryxyh commented Aug 6, 2018

Uh oh!

carryxyh commented Aug 7, 2018

Uh oh!

codecov-io commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carryxyh commented Aug 8, 2018

Uh oh!

chickenlj commented Aug 13, 2018

Uh oh!

zonghaishang commented Aug 13, 2018

Uh oh!

carryxyh commented Aug 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Aug 7, 2018 •

edited

Loading