IGNITE-23200 applyOnDefaultChannel logic fixes #11660
Conversation
| @@ -703,7 +703,8 @@ synchronized void initChannelHolders() { | |||
| curAddrs.putIfAbsent(addr, hld); | |||
| } | |||
|
|
|||
| reinitHolders.add(hld); | |||
| if (!reinitHolders.contains(hld)) | |||
There was a problem hiding this comment.
In most cases ArrayList#contains will traverse whole collection just to find that no such holder. Let's make the check more optimal.
| int fixedAttemptsLimit = attemptsLimit; | ||
|
|
||
| // + 1 is only required if the failure list is empty/null | ||
| while (fixedAttemptsLimit + 1 > (failures == null ? 0 : failures.size())) { |
There was a problem hiding this comment.
Move +1 to var assignment.
| while (attemptsLimit > (failures == null ? 0 : failures.size())) { | ||
| int fixedAttemptsLimit = attemptsLimit; | ||
|
|
||
| // + 1 is only required if the failure list is empty/null |
| @@ -681,6 +682,8 @@ synchronized void initChannelHolders() { | |||
| if (idx != -1) | |||
| currDfltHolder = holders.get(idx); | |||
|
|
|||
| Set<ClientChannelHolder> uniqueHolders = new HashSet<>(); | |||
There was a problem hiding this comment.
Looks like, you don't need one more collection. You can base on existing logic:
- There is a loop over existing holders (see line 653)
- And there is a place where a new holder is created (see line 700)
| public class ServicesFailoverTest extends AbstractThinClientTest { | ||
| /** */ | ||
| @Test | ||
| public void test() throws Exception { |
There was a problem hiding this comment.
- Let's add the test to existing test class -
IgniteClientReconnectServicesTest. - Name of the method must be somehow meaningful.
| @@ -817,7 +819,11 @@ private <T> T applyOnDefaultChannel( | |||
| ClientOperation op, | |||
| @Nullable List<ClientConnectionException> failures | |||
| ) { | |||
| while (attemptsLimit > (failures == null ? 0 : failures.size())) { | |||
| // +1 is required for the correct channel search if ReliableChannel#applyOnDefaultChannel#idx selects | |||
There was a problem hiding this comment.
An additional attempt is needed because N+1 channels might be used for sending a message - first a random one, then each one from #channels in sequence.
There was a problem hiding this comment.
Wouldn't it be better to explain why this result of a random choice influences?
| /** Test to verify service failover after a node failure. */ | ||
| @Test | ||
| public void testServiceFailoverAfterNodeFailure() throws Exception { | ||
| String TEST_SRVC_NAME = "cluster-singleton-svc"; |
There was a problem hiding this comment.
Actually, you can use existing singleton service instead - CLUSTER_SINGLTON_SERVICE_NAME with TestServiceInterface.
FewJuho
force-pushed
the
ignite-23200
branch
from
75090e1
to
8af2615
Compare
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email [email protected] or ask anу advice on http://asf.slack.com #ignite channel.