You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "FewJuho (via GitHub)" <gi...@apache.org> on 2025/02/06 07:06:44 UTC
[PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
FewJuho opened a new pull request, #11855:
URL: https://github.com/apache/ignite/pull/11855
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
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
Posted by "FewJuho (via GitHub)" <gi...@apache.org>.
FewJuho commented on code in PR #11855:
URL: https://github.com/apache/ignite/pull/11855#discussion_r1967625605
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -944,6 +943,10 @@ private int getRetryLimit() {
int size = holders.size();
+ // Essential to produce a retry connection on the channel after a failure occurred on that single channel.
+ if (channelsCnt.get() == 0 && partitionAwarenessEnabled)
Review Comment:
Agreed, removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
Posted by "FewJuho (via GitHub)" <gi...@apache.org>.
FewJuho commented on code in PR #11855:
URL: https://github.com/apache/ignite/pull/11855#discussion_r1971846291
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -703,7 +704,8 @@ synchronized void initChannelHolders() {
curAddrs.putIfAbsent(addr, hld);
}
- reinitHolders.add(hld);
+ if (!reinitHolders.contains(hld))
Review Comment:
Since I rewrote validAddrsSet to be of type Set<List<InetSocketAddress>> instead of Set<InetSocketAddress>, this allowed us to restore as normal hld.setConfiguration and hld = new ClientChannelHolder. However, now there can be duplicate holders across different addresses, and we need to filter them to remove duplicates.
We can first make reinitHolders a Set, and after adding the holders, convert it back to a List.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
Posted by "timoninmaxim (via GitHub)" <gi...@apache.org>.
timoninmaxim merged PR #11855:
URL: https://github.com/apache/ignite/pull/11855
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #11855:
URL: https://github.com/apache/ignite/pull/11855#issuecomment-2741281910
## [](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=11855) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=11855&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=11855&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=11855&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=11855&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=11855&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=11855)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
Posted by "FewJuho (via GitHub)" <gi...@apache.org>.
FewJuho commented on code in PR #11855:
URL: https://github.com/apache/ignite/pull/11855#discussion_r1971852667
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -631,43 +631,44 @@ synchronized void initChannelHolders() {
return;
}
- // Add connected channels to the list to avoid unnecessary reconnects, unless address finder is used.
- if (holders != null && clientCfg.getAddressesFinder() == null) {
- // Do not modify the original list.
- newAddrs = new ArrayList<>(newAddrs);
-
- for (ClientChannelHolder h : holders) {
- ClientChannel ch = h.ch;
-
- if (ch != null && !ch.closed())
- newAddrs.add(h.getAddresses());
- }
- }
-
Map<InetSocketAddress, ClientChannelHolder> curAddrs = new HashMap<>();
- Set<InetSocketAddress> newAddrsSet = newAddrs.stream().flatMap(Collection::stream).collect(Collectors.toSet());
+ Set<List<InetSocketAddress>> validAddrsSet = new HashSet<>(newAddrs);
+ // Add connected channels to the list to avoid unnecessary reconnects, unless address finder is used.
// Close obsolete holders or map old but valid addresses to holders
if (holders != null) {
for (ClientChannelHolder h : holders) {
- boolean found = false;
+ ClientChannel ch = h.ch;
- for (InetSocketAddress addr : h.getAddresses()) {
- // If new endpoints contain at least one of channel addresses, don't close this channel.
- if (newAddrsSet.contains(addr)) {
- ClientChannelHolder oldHld = curAddrs.putIfAbsent(addr, h);
+ if (clientCfg.getAddressesFinder() == null && ch != null && !ch.closed()) {
+ validAddrsSet.add(h.getAddresses());
+ for (InetSocketAddress addr : h.getAddresses())
+ curAddrs.putIfAbsent(addr, h);
+ }
- if (oldHld == null || oldHld == h) // If not duplicate.
- found = true;
+ boolean found = false;
+
+ // If new endpoints contain at least one of channel addresses, don't close this channel.
+ for (List<InetSocketAddress> addrList : validAddrsSet) {
+ if (addrList.containsAll(h.getAddresses())) {
Review Comment:
It's not optimal; we can wrap it in "new HashSet<>(addrList).containsAll"
Is it worth it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
Posted by "FewJuho (via GitHub)" <gi...@apache.org>.
FewJuho commented on code in PR #11855:
URL: https://github.com/apache/ignite/pull/11855#discussion_r1967619468
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -656,14 +657,16 @@ synchronized void initChannelHolders() {
for (InetSocketAddress addr : h.getAddresses()) {
// If new endpoints contain at least one of channel addresses, don't close this channel.
if (newAddrsSet.contains(addr)) {
- ClientChannelHolder oldHld = curAddrs.putIfAbsent(addr, h);
-
- if (oldHld == null || oldHld == h) // If not duplicate.
- found = true;
+ found = true;
+ break;
}
}
- if (!found)
+ if (found) {
+ for (InetSocketAddress addr : h.getAddresses())
+ curAddrs.putIfAbsent(addr, h);
Review Comment:
These holders are absent in case clientCfg.getAddressesFinder() != null
Is it worth checking this way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
Posted by "timoninmaxim (via GitHub)" <gi...@apache.org>.
timoninmaxim commented on code in PR #11855:
URL: https://github.com/apache/ignite/pull/11855#discussion_r1966502279
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -656,14 +657,16 @@ synchronized void initChannelHolders() {
for (InetSocketAddress addr : h.getAddresses()) {
// If new endpoints contain at least one of channel addresses, don't close this channel.
if (newAddrsSet.contains(addr)) {
- ClientChannelHolder oldHld = curAddrs.putIfAbsent(addr, h);
-
- if (oldHld == null || oldHld == h) // If not duplicate.
- found = true;
+ found = true;
+ break;
}
}
- if (!found)
+ if (found) {
+ for (InetSocketAddress addr : h.getAddresses())
+ curAddrs.putIfAbsent(addr, h);
Review Comment:
Looks weird, you already put all this holders earlier, see line 644
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -678,29 +681,25 @@ synchronized void initChannelHolders() {
int idx = curChIdx;
- if (idx != -1)
+ if (idx != -1 && holders != null)
currDfltHolder = holders.get(idx);
- for (List<InetSocketAddress> addrs : newAddrs) {
- ClientChannelHolder hld = null;
+ for (InetSocketAddress addr : newAddrsSet) {
+ ClientChannelHolder hld = curAddrs.get(addr);
- // Try to find already created channel holder.
- for (InetSocketAddress addr : addrs) {
- hld = curAddrs.get(addr);
+ if (hld == null) {
+ List<InetSocketAddress> addrList = new ArrayList<>();
- if (hld != null) {
- if (!hld.getAddresses().equals(addrs)) // Enrich holder addresses.
- hld.setConfiguration(new ClientChannelConfiguration(clientCfg, addrs));
+ addrList.add(addr);
- break;
- }
+ hld = new ClientChannelHolder(new ClientChannelConfiguration(clientCfg, addrList));
}
+ else if (!hld.getAddresses().contains(addr)) {
+ List<InetSocketAddress> newAddrList = new ArrayList<>(hld.getAddresses());
Review Comment:
looks like already stored list can be reused.
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -631,22 +631,23 @@ synchronized void initChannelHolders() {
return;
}
+ Map<InetSocketAddress, ClientChannelHolder> curAddrs = new HashMap<>();
+
+ Set<InetSocketAddress> newAddrsSet = newAddrs.stream().flatMap(Collection::stream).collect(Collectors.toSet());
Review Comment:
Looks like it duplicates `curAddrs.keySet()`.
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -944,6 +943,10 @@ private int getRetryLimit() {
int size = holders.size();
+ // Essential to produce a retry connection on the channel after a failure occurred on that single channel.
+ if (channelsCnt.get() == 0 && partitionAwarenessEnabled)
Review Comment:
Looks like you don't need this change, we want fix duplication first in this patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
Posted by "FewJuho (via GitHub)" <gi...@apache.org>.
FewJuho commented on code in PR #11855:
URL: https://github.com/apache/ignite/pull/11855#discussion_r2000823401
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -918,8 +919,22 @@ private <T> T applyOnNodeChannelWithFallback(UUID tryNodeId, Function<ClientChan
try {
channel = hld.getOrCreateChannel();
+ try {
+ return function.apply(channel);
+ }
+ catch (ClientConnectionException e) {
+ if (partitionAwarenessEnabled) {
Review Comment:
Should we retry the connection in case of partitionAwarenessEnabled?
In the applyOnDefaultChannel reference method we take care of partitionAwarenessEnabled=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
Posted by "FewJuho (via GitHub)" <gi...@apache.org>.
FewJuho commented on code in PR #11855:
URL: https://github.com/apache/ignite/pull/11855#discussion_r1967644448
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -631,22 +631,23 @@ synchronized void initChannelHolders() {
return;
}
+ Map<InetSocketAddress, ClientChannelHolder> curAddrs = new HashMap<>();
+
+ Set<InetSocketAddress> newAddrsSet = newAddrs.stream().flatMap(Collection::stream).collect(Collectors.toSet());
Review Comment:
Since an addr needs to be added, explicit reuse is not possible, but it can be reused using streams
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -678,29 +681,25 @@ synchronized void initChannelHolders() {
int idx = curChIdx;
- if (idx != -1)
+ if (idx != -1 && holders != null)
currDfltHolder = holders.get(idx);
- for (List<InetSocketAddress> addrs : newAddrs) {
- ClientChannelHolder hld = null;
+ for (InetSocketAddress addr : newAddrsSet) {
+ ClientChannelHolder hld = curAddrs.get(addr);
- // Try to find already created channel holder.
- for (InetSocketAddress addr : addrs) {
- hld = curAddrs.get(addr);
+ if (hld == null) {
+ List<InetSocketAddress> addrList = new ArrayList<>();
- if (hld != null) {
- if (!hld.getAddresses().equals(addrs)) // Enrich holder addresses.
- hld.setConfiguration(new ClientChannelConfiguration(clientCfg, addrs));
+ addrList.add(addr);
- break;
- }
+ hld = new ClientChannelHolder(new ClientChannelConfiguration(clientCfg, addrList));
}
+ else if (!hld.getAddresses().contains(addr)) {
+ List<InetSocketAddress> newAddrList = new ArrayList<>(hld.getAddresses());
Review Comment:
Since an addr needs to be added, explicit reuse is not possible, but it can be reused using streams
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
Posted by "timoninmaxim (via GitHub)" <gi...@apache.org>.
timoninmaxim commented on code in PR #11855:
URL: https://github.com/apache/ignite/pull/11855#discussion_r2005987994
##########
modules/core/src/test/java/org/apache/ignite/client/ReliabilityTest.java:
##########
@@ -265,25 +265,45 @@ public void testRetryReadPolicyRetriesCacheGet() {
}
/**
- * Tests retry policy exception handling.
+ * Tests that retry policy exception handling for async operations propagates to the caller.
*/
@Test
- public void testExceptionInRetryPolicyPropagatesToCaller() {
+ public void testExceptionInRetryPolicyPropagatesToCallerAsync() {
Assume.assumeFalse(partitionAware);
try (LocalIgniteCluster cluster = LocalIgniteCluster.start(1);
IgniteClient client = Ignition.startClient(getClientConfiguration()
.setRetryPolicy(new ExceptionRetryPolicy())
- .setAddresses(F.first(cluster.clientAddresses()), F.first(cluster.clientAddresses()))
+ .setAddresses(F.first(cluster.clientAddresses()))
.setClusterDiscoveryEnabled(false))
) {
ClientCache<Integer, Integer> cache = client.createCache("cache");
+
dropAllThinClientConnections(Ignition.allGrids().get(0));
- Throwable asyncEx = GridTestUtils.assertThrows(null, () -> cache.getAsync(0).get(),
- ExecutionException.class, "Channel is closed");
+ Throwable asyncEx = GridTestUtils.assertThrows(null,
+ () -> cache.getAsync(0).get(),
+ ExecutionException.class, "Channel is closed");
GridTestUtils.assertContains(null, asyncEx.getMessage(), F.first(cluster.clientAddresses()));
+ }
+ }
+
Review Comment:
Excess white line
##########
modules/core/src/test/java/org/apache/ignite/client/ReliabilityTest.java:
##########
@@ -265,25 +265,45 @@ public void testRetryReadPolicyRetriesCacheGet() {
}
/**
- * Tests retry policy exception handling.
+ * Tests that retry policy exception handling for async operations propagates to the caller.
*/
@Test
- public void testExceptionInRetryPolicyPropagatesToCaller() {
+ public void testExceptionInRetryPolicyPropagatesToCallerAsync() {
Assume.assumeFalse(partitionAware);
try (LocalIgniteCluster cluster = LocalIgniteCluster.start(1);
IgniteClient client = Ignition.startClient(getClientConfiguration()
.setRetryPolicy(new ExceptionRetryPolicy())
- .setAddresses(F.first(cluster.clientAddresses()), F.first(cluster.clientAddresses()))
+ .setAddresses(F.first(cluster.clientAddresses()))
.setClusterDiscoveryEnabled(false))
) {
ClientCache<Integer, Integer> cache = client.createCache("cache");
+
dropAllThinClientConnections(Ignition.allGrids().get(0));
- Throwable asyncEx = GridTestUtils.assertThrows(null, () -> cache.getAsync(0).get(),
- ExecutionException.class, "Channel is closed");
+ Throwable asyncEx = GridTestUtils.assertThrows(null,
Review Comment:
No changes in this lines
##########
modules/core/src/test/java/org/apache/ignite/client/ReliabilityTest.java:
##########
@@ -265,25 +265,45 @@ public void testRetryReadPolicyRetriesCacheGet() {
}
/**
- * Tests retry policy exception handling.
+ * Tests that retry policy exception handling for async operations propagates to the caller.
*/
@Test
- public void testExceptionInRetryPolicyPropagatesToCaller() {
+ public void testExceptionInRetryPolicyPropagatesToCallerAsync() {
Review Comment:
use async test param instead
##########
modules/core/src/test/java/org/apache/ignite/internal/client/thin/ReliableChannelTest.java:
##########
@@ -132,16 +132,19 @@ private Set<String> usedDefaultChannels(String... addrs) {
* Checks that reinitialization of duplicated address is correct.
*/
@Test
- public void testReinitDuplicatedAddress() {
+ public void testReinitDuplicatedAddressMerging() {
Review Comment:
Let's not rename test to keep test history
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
Posted by "FewJuho (via GitHub)" <gi...@apache.org>.
FewJuho commented on code in PR #11855:
URL: https://github.com/apache/ignite/pull/11855#discussion_r1971846291
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -703,7 +704,8 @@ synchronized void initChannelHolders() {
curAddrs.putIfAbsent(addr, hld);
}
- reinitHolders.add(hld);
+ if (!reinitHolders.contains(hld))
Review Comment:
Since I rewrote validAddrsSet to be of type Set<List<InetSocketAddress>> instead of Set<InetSocketAddress>, this allowed us to restore as normal hld.setConfiguration and hld = new ClientChannelHolder. However, now there can be duplicate holders across different addresses, and we need to filter them to remove duplicates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23856 Ignite Thin Client channels unwanted duplication [ignite]
Posted by "timoninmaxim (via GitHub)" <gi...@apache.org>.
timoninmaxim commented on code in PR #11855:
URL: https://github.com/apache/ignite/pull/11855#discussion_r1991061911
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -678,7 +675,7 @@ synchronized void initChannelHolders() {
int idx = curChIdx;
- if (idx != -1)
+ if (idx != -1 && holders != null)
Review Comment:
Looks like, that in case when `idx != -1` then `holders` never `null`. Could you please describe the scenario?
##########
modules/core/src/test/java/org/apache/ignite/client/ReliabilityTest.java:
##########
@@ -259,42 +265,67 @@ public void testRetryReadPolicyRetriesCacheGet() {
// Fail.
dropAllThinClientConnections(Ignition.allGrids().get(0));
- // Reuse second address without fail.
+ if (!partitionAware) {
+ Throwable ex = GridTestUtils.assertThrowsWithCause(() -> cache.get(0), ClientConnectionException.class);
+
+ GridTestUtils.assertContains(null, ex.getMessage(), F.first(cluster.clientAddresses()));
+ }
+
+ // Reuse the address after recovery without fail.
cache.get(0);
}
}
/**
- * Tests retry policy exception handling.
+ * Tests that retry policy exception handling for async operations propagates to the caller.
*/
@Test
- public void testExceptionInRetryPolicyPropagatesToCaller() {
+ public void testExceptionInRetryPolicyPropagatesToCallerAsync() {
Assume.assumeFalse(partitionAware);
try (LocalIgniteCluster cluster = LocalIgniteCluster.start(1);
IgniteClient client = Ignition.startClient(getClientConfiguration()
.setRetryPolicy(new ExceptionRetryPolicy())
- .setAddresses(F.first(cluster.clientAddresses()), F.first(cluster.clientAddresses()))
+ .setAddresses(F.first(cluster.clientAddresses()))
.setClusterDiscoveryEnabled(false))
) {
ClientCache<Integer, Integer> cache = client.createCache("cache");
+
dropAllThinClientConnections(Ignition.allGrids().get(0));
- Throwable asyncEx = GridTestUtils.assertThrows(null, () -> cache.getAsync(0).get(),
- ExecutionException.class, "Channel is closed");
+ Throwable asyncEx = GridTestUtils.assertThrows(null,
+ () -> cache.getAsync(0).get(),
+ ExecutionException.class, "Channel is closed");
GridTestUtils.assertContains(null, asyncEx.getMessage(), F.first(cluster.clientAddresses()));
+ }
+ }
+
+
+ /**
+ * Tests that retry policy exception handling for sync operations propagates to the caller.
+ */
+ @Test
+ public void testExceptionInRetryPolicyPropagatesToCallerSync() {
+ Assume.assumeFalse(partitionAware);
+
+ try (LocalIgniteCluster cluster = LocalIgniteCluster.start(1);
+ IgniteClient client = Ignition.startClient(getClientConfiguration()
+ .setRetryPolicy(new ExceptionRetryPolicy())
+ .setAddresses(F.first(cluster.clientAddresses()))
+ .setClusterDiscoveryEnabled(false))
+ ) {
+ ClientCache<Integer, Integer> cache = client.createCache("cache");
dropAllThinClientConnections(Ignition.allGrids().get(0));
- Throwable syncEx = GridTestUtils.assertThrows(null, () -> cache.get(0),
+ Throwable syncEx = GridTestUtils.assertThrows(null,
Review Comment:
useless change
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -918,8 +919,22 @@ private <T> T applyOnNodeChannelWithFallback(UUID tryNodeId, Function<ClientChan
try {
channel = hld.getOrCreateChannel();
+ try {
Review Comment:
Add empty line before try
##########
modules/core/src/test/java/org/apache/ignite/internal/client/thin/ReliableChannelDuplicationTest.java:
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.client.thin;
+
+import java.net.InetSocketAddress;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.junit.Assume;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static java.util.stream.IntStream.range;
+
+/**
+ * Tests for duplication in channels' list.
+ */
+@RunWith(Parameterized.class)
+public class ReliableChannelDuplicationTest extends ThinClientAbstractPartitionAwarenessTest {
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ super.afterTest();
+
+ stopAllGrids();
+ }
+
+ /** Grid count. */
+ @Parameterized.Parameter(0)
+ public int gridCnt;
+
+ /** */
+ @Parameterized.Parameters(name = "gridCount = {0}")
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ { 1 },
+ { 3 }
+ });
+ }
+
+ /**
+ * Test after cluster restart the number of channels remains equal to the number of nodes.
+ */
+ @Test
+ public void testDuplicationOnClusterRestart() throws Exception {
+ startGrids(gridCnt);
+
+ initClient(getClientConfiguration(range(0, gridCnt).toArray()), range(0, gridCnt).toArray());
+
+ assertNoDuplicates(((TcpIgniteClient)client).reliableChannel().getChannelHolders());
+
+ stopAllGrids();
+
+ startGrids(gridCnt);
+
+ client.cacheNames();
+
+ assertNoDuplicates(((TcpIgniteClient)client).reliableChannel().getChannelHolders());
+ }
+
+ /**
+ * Test behavior after stopping a single node in the cluster.
+ */
+ @Test
+ public void testStopSingleNodeDuringOperation() throws Exception {
+ Assume.assumeFalse(gridCnt == 1);
+
+ testChannelDuplication(gridCnt, 1, 0);
+ }
+
+ /**
+ * Test behavior after stopping and restarting a node.
+ */
+ @Test
+ public void testStopAndRestartNode() throws Exception {
+ Assume.assumeFalse(gridCnt == 1);
+
+ testChannelDuplication(gridCnt, 1, 1);
+ }
+
+ /**
+ * Test behavior after stopping multiple nodes in the cluster.
+ */
+ @Test
+ public void testStopMultipleNodesDuringOperation() throws Exception {
+ Assume.assumeFalse(gridCnt < 3);
+
+ testChannelDuplication(gridCnt, 2, 2);
+ }
+
+ /**
+ * Asserts that there are no duplicate channels in the list of holders based on their remote addresses.
+ *
+ * @param holders List of channel holders.
+ */
+ private void assertNoDuplicates(List<ReliableChannel.ClientChannelHolder> holders) {
+ Set<InetSocketAddress> addrs = new HashSet<>();
+
+ for (ReliableChannel.ClientChannelHolder holder : holders) {
+ holder.getAddresses().forEach(addr -> {
+ if (!addrs.add(addr))
+ throw new AssertionError("Duplicate remote address found: " + addr);
+ });
+ }
+ }
+
+ /**
+ * Stop a Node and provide an operation to notify the client about new topology.
+ */
+ private void stopNodeAndMakeTopologyChangeDetection(int idx) {
+ stopGrid(idx);
+
+ detectTopologyChange();
+
+ client.cacheNames();
+ }
+
+ /**
+ * Tests that no duplicate channel holders are created during node restarts and topology changes.
+ *
+ * @param gridCnt int Grids to start.
+ * @param gridsStop int Grids to stop.
+ * @param gridsRestart int Grids to restart after stop.
+ */
+ private void testChannelDuplication(int gridCnt, int gridsStop, int gridsRestart) throws Exception {
Review Comment:
`gridCnt` is already a class level param.
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -931,7 +946,6 @@ private <T> T applyOnNodeChannelWithFallback(UUID tryNodeId, Function<ClientChan
throw e;
}
}
-
Review Comment:
Remove useless change
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ReliableChannel.java:
##########
@@ -918,8 +919,22 @@ private <T> T applyOnNodeChannelWithFallback(UUID tryNodeId, Function<ClientChan
try {
channel = hld.getOrCreateChannel();
+ try {
+ return function.apply(channel);
+ }
+ catch (ClientConnectionException e) {
+ if (partitionAwarenessEnabled) {
Review Comment:
Why do we care about `partitionAwarenessEnabled` here?
##########
modules/core/src/test/java/org/apache/ignite/internal/client/thin/ReliableChannelDuplicationTest.java:
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.client.thin;
+
+import java.net.InetSocketAddress;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.junit.Assume;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static java.util.stream.IntStream.range;
+
+/**
+ * Tests for duplication in channels' list.
+ */
+@RunWith(Parameterized.class)
+public class ReliableChannelDuplicationTest extends ThinClientAbstractPartitionAwarenessTest {
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ super.afterTest();
+
+ stopAllGrids();
+ }
+
+ /** Grid count. */
+ @Parameterized.Parameter(0)
+ public int gridCnt;
+
+ /** */
+ @Parameterized.Parameters(name = "gridCount = {0}")
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ { 1 },
+ { 3 }
+ });
+ }
+
+ /**
+ * Test after cluster restart the number of channels remains equal to the number of nodes.
+ */
+ @Test
+ public void testDuplicationOnClusterRestart() throws Exception {
+ startGrids(gridCnt);
+
+ initClient(getClientConfiguration(range(0, gridCnt).toArray()), range(0, gridCnt).toArray());
+
+ assertNoDuplicates(((TcpIgniteClient)client).reliableChannel().getChannelHolders());
+
+ stopAllGrids();
+
+ startGrids(gridCnt);
+
+ client.cacheNames();
Review Comment:
Which guarantee do you want get from this call?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org