Do not retry TargetNotMemberException when invocation made on target #10405
Conversation
|
run-lab-run |
|
Test PASSed. |
| @@ -182,6 +184,11 @@ public void notifyException(Throwable exception) { | |||
| return; | |||
| } | |||
|
|
|||
| if (address != null && exception instanceof TargetNotMemberException) { | |||
There was a problem hiding this comment.
This will prevent all invocations which receive TargetNotMemberException being not retried even when it is actually retryable and it was being retried before. E.g. IMap.get() will end up at invocationService.invokeOnTarget() where address != null. Don't we want to retry these invocations?
There was a problem hiding this comment.
My thinking was as following:
TargetNotMemberException is thrown when address is not in memberlist. It can be thrown from either server or client.
If invocation intended to be send to an address (address != null), it will retry always in same address. If address is not in memberlist, there is no point retrying. It does not matter if client message is retyable or not it will never be executed.
Makes sense ?
Note: IMap.get does not use invokeOnTarget, it uses invokeOnPartition
| @@ -171,6 +172,7 @@ public void notify(ClientMessage clientMessage) { | |||
| clientInvocationFuture.complete(clientMessage); | |||
| } | |||
|
|
|||
| @SuppressWarnings({ "checkstyle:cyclomaticcomplexity", "checkstyle:npathcomplexity" }) | |||
There was a problem hiding this comment.
is there no workaround not to introduce this suppression?
There was a problem hiding this comment.
The workaround would be to simplify the method, so less branches etc. (normally you can achieve this by pulling out methods).
There was a problem hiding this comment.
I am not in favor of pulling out methods when methods are not reused. But I have a pr cookinf up built on top of this. It will simplify these parts by introducing invocation classes for different targets. There I will remove back the suppressions.
sancar
force-pushed
the
fix/targetNotMember/master
branch
from
b6e688b
to
436446a
Compare
|
Test PASSed. |
sancar
force-pushed
the
fix/targetNotMember/master
branch
from
436446a
to
8c8e1c5
Compare
Invocation made `ProxyManager.initialize(ClientProxy)` was on connection to be able to prevent it from retrying. It was a workaround that does not comply with ClientInvocation API hence failed today. In this pr, I made the proxy creation invocation on address as intended. And I changed invocation exception handling so that it will not be retried when given address not a member anymore. Retrying does not make sense in this case. ClientLockWithTerminationTest.testLockOnClient_withNodeCrash was failing because it was retrying creating proxy on closed member until clientInvocationTimeout(120 seconds) hit and then throws OperationTimeoutException. This pr hazelcast#10370 made issue apparent. It was waiting for 120 seconds unncessarily, then throwing a retryableException(instead OperationTimeoutException). Because of retyable exception there issue was hidden. fixes hazelcast#3422
|
Test PASSed. |
Invocation made
ProxyManager.initialize(ClientProxy)was on connectionto be able to prevent it from retrying. It was a workaround that does not
comply with ClientInvocation API hence failed today.
In this pr, I made the proxy creation invocation on address as intended.
And I changed invocation exception handling so that it will not be retried
when given address not a member anymore. Retrying does not make sense in
this case.
ClientLockWithTerminationTest.testLockOnClient_withNodeCrash was failing
because it was retrying creating proxy on closed member until
clientInvocationTimeout(120 seconds) hit and then throws OperationTimeoutException.
This pr #10370 made issue apparent.
It was waiting for 120 seconds unncessarily, then throwing a
retryableException(instead OperationTimeoutException). Because of retyable
exception there issue was hidden.
fixes #3422