From 013dd2fdef7603a594ae67ad2fbc38dc174b2279 Mon Sep 17 00:00:00 2001 From: nicktorwald Date: Wed, 12 Jun 2019 01:46:04 +0700 Subject: [PATCH 1/2] Do not use call_16 by default The call_16 became obsolete when tarantool-1.6 was gone. There're no reasons to continue using the old version by default. Change discovery function requirements in part of single value support. This is done to be consistent with other Tarantool connectors. Closes: #196 --- README.md | 3 +-- .../java/org/tarantool/AbstractTarantoolOps.java | 2 +- .../java/org/tarantool/TarantoolClientConfig.java | 8 +++++--- src/main/java/org/tarantool/TarantoolClientImpl.java | 10 +++++----- .../TarantoolClusterStoredFunctionDiscoverer.java | 2 +- .../java/org/tarantool/ClientReconnectClusterIT.java | 2 +- .../ClusterServiceStoredFunctionDiscovererIT.java | 12 ++++++++++++ 7 files changed, 26 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index d3a07fde..ac04fb16 100644 --- a/README.md +++ b/README.md @@ -237,8 +237,7 @@ tarantool> function get_cluster_nodes() return { 'host1:3301', 'host2:3302', 'ho You need to pay attention to a function contract we are currently supporting: * The client never passes any arguments to a discovery function. -* A discovery function _should_ return a single result of strings (i.e. single - string `return 'host:3301'` or array of strings `return {'host1:3301', 'host2:3301'}`). +* A discovery function _must_ return an array of strings (i.e `return {'host1:3301', 'host2:3301'}`). * A discovery function _may_ return multi-results but the client takes into account only first of them (i.e. `return {'host:3301'}, discovery_delay`, where the second result is unused). Even more, any extra results __are reserved__ by the client diff --git a/src/main/java/org/tarantool/AbstractTarantoolOps.java b/src/main/java/org/tarantool/AbstractTarantoolOps.java index 3d926155..8cefb379 100644 --- a/src/main/java/org/tarantool/AbstractTarantoolOps.java +++ b/src/main/java/org/tarantool/AbstractTarantoolOps.java @@ -4,7 +4,7 @@ public abstract class AbstractTarantoolOps implements TarantoolClientOps { - private Code callCode = Code.OLD_CALL; + private Code callCode = Code.CALL; protected abstract Result exec(Code code, Object... args); diff --git a/src/main/java/org/tarantool/TarantoolClientConfig.java b/src/main/java/org/tarantool/TarantoolClientConfig.java index bba14301..c10c7857 100644 --- a/src/main/java/org/tarantool/TarantoolClientConfig.java +++ b/src/main/java/org/tarantool/TarantoolClientConfig.java @@ -44,10 +44,12 @@ public class TarantoolClientConfig { public long writeTimeoutMillis = 60 * 1000L; /** - * Use old call command https://github.com/tarantool/doc/issues/54, - * please ensure that you server supports new call command. + * Use new call method instead of obsolete + * {@code call_16} which used to work in Tarantool v1.6. + * + * Since 1.9.3, this flag has become enabled by default */ - public boolean useNewCall = false; + public boolean useNewCall = true; /** * Max time to establish connection to the server diff --git a/src/main/java/org/tarantool/TarantoolClientImpl.java b/src/main/java/org/tarantool/TarantoolClientImpl.java index 3bebbc82..eed09a45 100644 --- a/src/main/java/org/tarantool/TarantoolClientImpl.java +++ b/src/main/java/org/tarantool/TarantoolClientImpl.java @@ -115,11 +115,11 @@ private void initClient(SocketChannelProvider socketProvider, TarantoolClientCon this.syncOps = new SyncOps(); this.composableAsyncOps = new ComposableAsyncOps(); this.fireAndForgetOps = new FireAndForgetOps(); - if (config.useNewCall) { - setCallCode(Code.CALL); - this.syncOps.setCallCode(Code.CALL); - this.fireAndForgetOps.setCallCode(Code.CALL); - this.composableAsyncOps.setCallCode(Code.CALL); + if (!config.useNewCall) { + setCallCode(Code.OLD_CALL); + this.syncOps.setCallCode(Code.OLD_CALL); + this.fireAndForgetOps.setCallCode(Code.OLD_CALL); + this.composableAsyncOps.setCallCode(Code.OLD_CALL); } } diff --git a/src/main/java/org/tarantool/cluster/TarantoolClusterStoredFunctionDiscoverer.java b/src/main/java/org/tarantool/cluster/TarantoolClusterStoredFunctionDiscoverer.java index c25b578d..05c7e89f 100644 --- a/src/main/java/org/tarantool/cluster/TarantoolClusterStoredFunctionDiscoverer.java +++ b/src/main/java/org/tarantool/cluster/TarantoolClusterStoredFunctionDiscoverer.java @@ -55,7 +55,7 @@ private void checkResult(List result) { if (result == null || result.isEmpty()) { throw new IllegalDiscoveryFunctionResult("Discovery function returned no data"); } - if (!((List)result.get(0)).stream().allMatch(item -> item instanceof String)) { + if (!(result.get(0) instanceof List)) { throw new IllegalDiscoveryFunctionResult("The first value must be an array of strings"); } } diff --git a/src/test/java/org/tarantool/ClientReconnectClusterIT.java b/src/test/java/org/tarantool/ClientReconnectClusterIT.java index 39e57727..c0228874 100644 --- a/src/test/java/org/tarantool/ClientReconnectClusterIT.java +++ b/src/test/java/org/tarantool/ClientReconnectClusterIT.java @@ -359,7 +359,7 @@ void testDelayFunctionResultFetch() { instances.get(SRV1) .executeLua("co = coroutine.create(function() " + functionBody + " end)"); instances.get(SRV1) - .executeLua("function getAddressesFunction() local c, r = coroutine.resume(co); return r end"); + .executeLua("function getAddressesFunction() local c, r = coroutine.resume(co); return {r} end"); String infoFunctionScript = makeDiscoveryFunction(infoFunctionName, Collections.singletonList(service3Address)); instances.get(SRV2).executeLua(infoFunctionScript); diff --git a/src/test/java/org/tarantool/cluster/ClusterServiceStoredFunctionDiscovererIT.java b/src/test/java/org/tarantool/cluster/ClusterServiceStoredFunctionDiscovererIT.java index d269578f..0c03c705 100644 --- a/src/test/java/org/tarantool/cluster/ClusterServiceStoredFunctionDiscovererIT.java +++ b/src/test/java/org/tarantool/cluster/ClusterServiceStoredFunctionDiscovererIT.java @@ -153,6 +153,18 @@ public void testWrongTypeResultData() { assertThrows(IllegalDiscoveryFunctionResult.class, discoverer::getInstances); } + @Test + @DisplayName("fetched with an exception when a single string returned") + public void testSingleStringResultData() { + String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, "'host1:3301'"); + control.openConsole(INSTANCE_NAME).exec(functionCode); + + TarantoolClusterStoredFunctionDiscoverer discoverer = + new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); + + assertThrows(IllegalDiscoveryFunctionResult.class, discoverer::getInstances); + } + @Test @DisplayName("fetched with an exception using no return function") public void testFunctionWithNoReturn() { From 522491455feb7554907710a1811d23afcccb2d56 Mon Sep 17 00:00:00 2001 From: nicktorwald Date: Wed, 12 Jun 2019 01:47:07 +0700 Subject: [PATCH 2/2] Filter wrong formatted discovery addresses Instead of discarding the full list of addresses when at least one address does not match the format like host[:port] a discoverer just skips the broken address and carries on processing. Closes: #195 --- README.md | 6 ++ ...antoolClusterStoredFunctionDiscoverer.java | 48 ++++++++--- .../tarantool/ClientAsyncOperationsIT.java | 3 +- .../org/tarantool/TarantoolClientOpsIT.java | 2 +- ...sterServiceStoredFunctionDiscovererIT.java | 79 ++++++++++++++++--- 5 files changed, 117 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index ac04fb16..025726e9 100644 --- a/README.md +++ b/README.md @@ -238,6 +238,10 @@ tarantool> function get_cluster_nodes() return { 'host1:3301', 'host2:3302', 'ho You need to pay attention to a function contract we are currently supporting: * The client never passes any arguments to a discovery function. * A discovery function _must_ return an array of strings (i.e `return {'host1:3301', 'host2:3301'}`). +* Each string _should_ satisfy the following pattern `host[:port]` + (or more strictly `/^[^:]+(:\d+)?$/` - a mandatory host containing any string + and an optional colon followed by digits of the port). Also, the port must be + in a range between 1 and 65535 if one is presented. * A discovery function _may_ return multi-results but the client takes into account only first of them (i.e. `return {'host:3301'}, discovery_delay`, where the second result is unused). Even more, any extra results __are reserved__ by the client @@ -270,6 +274,8 @@ client.syncOps().insert(45, Arrays.asList(1, 1)); * A discovery task always uses an active client connection to get the nodes list. It's in your responsibility to provide a function availability as well as a consistent nodes list on all instances you initially set or obtain from the task. +* Every address which is unmatched with `host[:port]` pattern will be filtered out from + the target addresses list. * If some error occurs while a discovery task is running then this task will be aborted without any after-effects for next task executions. These cases, for instance, are a wrong function result (see discovery function contract) or a broken connection. diff --git a/src/main/java/org/tarantool/cluster/TarantoolClusterStoredFunctionDiscoverer.java b/src/main/java/org/tarantool/cluster/TarantoolClusterStoredFunctionDiscoverer.java index 05c7e89f..da92cd3a 100644 --- a/src/main/java/org/tarantool/cluster/TarantoolClusterStoredFunctionDiscoverer.java +++ b/src/main/java/org/tarantool/cluster/TarantoolClusterStoredFunctionDiscoverer.java @@ -3,6 +3,7 @@ import org.tarantool.TarantoolClient; import org.tarantool.TarantoolClientOps; import org.tarantool.TarantoolClusterClientConfig; +import org.tarantool.util.StringUtils; import java.util.LinkedHashSet; import java.util.List; @@ -36,28 +37,57 @@ public Set getInstances() { // validation against the data returned; // this strict-mode allows us to extend the contract in a non-breaking // way for old clients just reserve an extra return value in - // terms of LUA multi-result support. - checkResult(list); - - List funcResult = (List) list.get(0); - return funcResult.stream() - .map(Object::toString) - .collect(Collectors.toCollection(LinkedHashSet::new)); + // terms of Lua multi-result support.; + return checkAndFilterAddresses(list); } /** * Check whether the result follows the contract or not. - * The contract is a mandatory single array of strings + * The contract is a mandatory single array of strings. + * + * The simplified format for each string is host[:port]. * * @param result result to be validated */ - private void checkResult(List result) { + private Set checkAndFilterAddresses(List result) { if (result == null || result.isEmpty()) { throw new IllegalDiscoveryFunctionResult("Discovery function returned no data"); } if (!(result.get(0) instanceof List)) { throw new IllegalDiscoveryFunctionResult("The first value must be an array of strings"); } + + return ((List) result.get(0)).stream() + .filter(item -> item instanceof String) + .map(Object::toString) + .filter(s -> !StringUtils.isBlank(s)) + .filter(this::isAddress) + .collect(Collectors.toCollection(LinkedHashSet::new)); + } + + /** + * Checks that address matches host[:port] format. + * + * @param address to be checked + * @return true if address follows the format + */ + private boolean isAddress(String address) { + if (address.endsWith(":")) { + return false; + } + String[] addressParts = address.split(":"); + if (addressParts.length > 2) { + return false; + } + if (addressParts.length == 2) { + try { + int port = Integer.parseInt(addressParts[1]); + return (port > 0 && port < 65536); + } catch (NumberFormatException e) { + return false; + } + } + return true; } } diff --git a/src/test/java/org/tarantool/ClientAsyncOperationsIT.java b/src/test/java/org/tarantool/ClientAsyncOperationsIT.java index 2f284c2c..087a4760 100644 --- a/src/test/java/org/tarantool/ClientAsyncOperationsIT.java +++ b/src/test/java/org/tarantool/ClientAsyncOperationsIT.java @@ -180,8 +180,7 @@ void testCall(AsyncOpsProvider provider) throws ExecutionException, InterruptedE testHelper.executeLua("function echo(...) return ... end"); Future> fut = provider.getAsyncOps().call("echo", "hello"); - assertEquals(Collections.singletonList(Collections.singletonList("hello")), - fut.get(TIMEOUT, TimeUnit.MILLISECONDS)); + assertEquals(Collections.singletonList("hello"), fut.get(TIMEOUT, TimeUnit.MILLISECONDS)); provider.close(); } diff --git a/src/test/java/org/tarantool/TarantoolClientOpsIT.java b/src/test/java/org/tarantool/TarantoolClientOpsIT.java index 59480b7a..ebaca9f5 100644 --- a/src/test/java/org/tarantool/TarantoolClientOpsIT.java +++ b/src/test/java/org/tarantool/TarantoolClientOpsIT.java @@ -489,7 +489,7 @@ public void testEval(SyncOpsProvider provider) { @MethodSource("getClientOps") public void testCall(SyncOpsProvider provider) { assertEquals( - Collections.singletonList(Collections.singletonList("true")), + Collections.singletonList("true"), provider.getClientOps().call("echo", "true") ); diff --git a/src/test/java/org/tarantool/cluster/ClusterServiceStoredFunctionDiscovererIT.java b/src/test/java/org/tarantool/cluster/ClusterServiceStoredFunctionDiscovererIT.java index 0c03c705..4e940033 100644 --- a/src/test/java/org/tarantool/cluster/ClusterServiceStoredFunctionDiscovererIT.java +++ b/src/test/java/org/tarantool/cluster/ClusterServiceStoredFunctionDiscovererIT.java @@ -1,6 +1,7 @@ package org.tarantool.cluster; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -23,6 +24,7 @@ import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Set; @@ -72,7 +74,7 @@ public void testSuccessfulAddressParsing() { testHelper.executeLua(functionCode); TarantoolClusterStoredFunctionDiscoverer discoverer = - new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); + new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); Set instances = discoverer.getInstances(); @@ -91,7 +93,7 @@ public void testSuccessfulUniqueAddressParsing() { testHelper.executeLua(functionCode); TarantoolClusterStoredFunctionDiscoverer discoverer = - new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); + new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); Set instances = discoverer.getInstances(); @@ -110,7 +112,7 @@ public void testFunctionReturnedEmptyList() { testHelper.executeLua(functionCode); TarantoolClusterStoredFunctionDiscoverer discoverer = - new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); + new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); Set instances = discoverer.getInstances(); @@ -124,7 +126,7 @@ public void testWrongFunctionName() { clusterConfig.clusterDiscoveryEntryFunction = "wrongFunction"; TarantoolClusterStoredFunctionDiscoverer discoverer = - new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); + new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); assertThrows(TarantoolException.class, discoverer::getInstances); } @@ -136,7 +138,7 @@ public void testWrongInstanceAddress() { client.close(); TarantoolClusterStoredFunctionDiscoverer discoverer = - new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); + new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); assertThrows(CommunicationException.class, discoverer::getInstances); } @@ -148,7 +150,7 @@ public void testWrongTypeResultData() { testHelper.executeLua(functionCode); TarantoolClusterStoredFunctionDiscoverer discoverer = - new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); + new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); assertThrows(IllegalDiscoveryFunctionResult.class, discoverer::getInstances); } @@ -157,7 +159,7 @@ public void testWrongTypeResultData() { @DisplayName("fetched with an exception when a single string returned") public void testSingleStringResultData() { String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, "'host1:3301'"); - control.openConsole(INSTANCE_NAME).exec(functionCode); + testHelper.executeLua(functionCode); TarantoolClusterStoredFunctionDiscoverer discoverer = new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); @@ -184,7 +186,7 @@ public void testWrongMultiResultData() { testHelper.executeLua(functionCode); TarantoolClusterStoredFunctionDiscoverer discoverer = - new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); + new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); Set instances = discoverer.getInstances(); @@ -200,9 +202,68 @@ public void testFunctionWithError() { testHelper.executeLua(functionCode); TarantoolClusterStoredFunctionDiscoverer discoverer = - new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); + new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); assertThrows(TarantoolException.class, discoverer::getInstances); } + @Test + @DisplayName("fetched a subset of valid addresses") + public void testFilterBadAddressesData() { + final List allHosts = Arrays.asList( + "host1:3313", + "host:abc", + "192.168.33.90", + "myHost", + "10.30.10.4:7814", + "host:311:sub-host", + "instance-2:", + "host:0", + "host:321981" + ); + + final Set expectedFiltered = new HashSet<>( + Arrays.asList( + "host1:3313", + "192.168.33.90", + "myHost", + "10.30.10.4:7814" + ) + ); + + String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, allHosts); + testHelper.executeLua(functionCode); + + TarantoolClusterStoredFunctionDiscoverer discoverer = + new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); + + Set instances = discoverer.getInstances(); + + assertNotNull(instances); + assertFalse(instances.isEmpty()); + assertEquals(expectedFiltered, instances); + } + + @Test + @DisplayName("fetched an empty set after filtration") + public void testFullBrokenAddressesList() { + List allHosts = Arrays.asList( + "abc:edf", + "192.168.33.90:", + "host:-123", + "host:0" + ); + + String functionCode = makeDiscoveryFunction(ENTRY_FUNCTION_NAME, allHosts); + testHelper.executeLua(functionCode); + + TarantoolClusterStoredFunctionDiscoverer discoverer = + new TarantoolClusterStoredFunctionDiscoverer(clusterConfig, client); + + Set instances = discoverer.getInstances(); + + assertNotNull(instances); + assertTrue(instances.isEmpty()); + } + }