sashapolo commented on a change in pull request #294: URL: https://github.com/apache/ignite-3/pull/294#discussion_r712108473
########## File path: modules/api/src/main/java/org/apache/ignite/configuration/schemas/network/ScaleCubeConfigurationSchema.java ########## @@ -0,0 +1,47 @@ +/* + * 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.configuration.schemas.network; + +import org.apache.ignite.configuration.annotation.Config; +import org.apache.ignite.configuration.annotation.Value; + +/** + * ScaleCube configuration. + */ +@Config +public class ScaleCubeConfigurationSchema { Review comment: All members of this class deserve a more detailed javadoc ########## File path: modules/api/src/main/java/org/apache/ignite/configuration/schemas/network/ClientConfigurationSchema.java ########## @@ -0,0 +1,41 @@ +/* + * 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.configuration.schemas.network; + +import org.apache.ignite.configuration.annotation.Config; +import org.apache.ignite.configuration.annotation.Value; +import org.apache.ignite.configuration.validation.Max; +import org.apache.ignite.configuration.validation.Min; + +/** Client socket configuration. See <a href="https://man7.org/linux/man-pages/man7/tcp.7.html";>TCP docs</a>. */ Review comment: actually, 2 of the 3 options are described [here](https://man7.org/linux/man-pages/man7/socket.7.html) ########## File path: modules/cli/src/integrationTest/java/org/apache/ignite/cli/ITConfigCommandTest.java ########## @@ -173,7 +188,23 @@ private void resetStreams() { out.reset(); } + /** + * Unescapes quotes in the input string. + * + * @param input String. + * @return String with unescaped quotes. + */ private String unescapeQuotes(String input) { Review comment: ```suggestion private static String unescapeQuotes(String input) { ``` ########## File path: modules/cli/src/integrationTest/java/org/apache/ignite/cli/ITConfigCommandTest.java ########## @@ -173,7 +188,23 @@ private void resetStreams() { out.reset(); } + /** + * Unescapes quotes in the input string. + * + * @param input String. + * @return String with unescaped quotes. + */ private String unescapeQuotes(String input) { return input.replace("\\\"", "\""); } + + /** + * Removes trailing quotes from the input string. + * + * @param input String. + * @return String without trailing quotes. + */ + private String removeTrailingQuotes(String input) { Review comment: ```suggestion private static String removeTrailingQuotes(String input) { ``` ########## File path: modules/network/src/main/java/org/apache/ignite/internal/network/netty/ConnectionManager.java ########## @@ -303,24 +305,24 @@ public String consistentId() { } /** - * Creates a {@link Bootstrap} for clients, providing channel handlers and options. + * Creates a {@link Bootstrap} for clients with channel options provided by {@link ClientView}. Review comment: ```suggestion * Creates a {@link Bootstrap} for clients with channel options provided by a {@link ClientView}. ``` ########## File path: modules/network/src/main/java/org/apache/ignite/internal/network/netty/NettyServer.java ########## @@ -260,6 +254,29 @@ public NettyServer( } } + /** + * Try bind this server to the port. + * + * @param port Target port. + * @param endPort Last port that server can be bound to. + * @param fut Future. + */ + private void tryBind(int port, int endPort, CompletableFuture<Channel> fut) { + if (port > endPort) + fut.completeExceptionally(new IllegalStateException("No available port in range")); + + NettyUtils.toChannelCompletableFuture(bootstrap.bind(port)).handle((channel, throwable) -> { Review comment: also, `whenComplete` is more suitable here ########## File path: modules/api/src/main/java/org/apache/ignite/configuration/schemas/network/ServerConfigurationSchema.java ########## @@ -0,0 +1,61 @@ +/* + * 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.configuration.schemas.network; + +import org.apache.ignite.configuration.annotation.Config; +import org.apache.ignite.configuration.annotation.Value; +import org.apache.ignite.configuration.validation.Max; +import org.apache.ignite.configuration.validation.Min; + +/** Server socket configuration. See <a href="https://man7.org/linux/man-pages/man7/tcp.7.html";>TCP docs</a>. */ Review comment: Same stuff about the link ########## File path: modules/api/src/main/java/org/apache/ignite/configuration/schemas/network/NodeFinderConfigurationSchema.java ########## @@ -0,0 +1,41 @@ +/* + * 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.configuration.schemas.network; + +import org.apache.ignite.configuration.annotation.Config; +import org.apache.ignite.configuration.annotation.Value; + +/** Node finder configuration. */ +@Config +public class NodeFinderConfigurationSchema { + /** Node finder type. */ + @Value(hasDefault = true) + public final String type = NodeFinderType.STATIC.name(); + + /** Cluster nodes. */ + @Value(hasDefault = true) + public final String[] netClusterNodes = new String[0]; + + /** Port start. */ + @Value(hasDefault = true) + public final int portStart = -1; Review comment: Should `portStart` and `portEnd` be annotated with `@Min(0)`? ########## File path: modules/api/src/main/java/org/apache/ignite/configuration/schemas/network/NodeFinderConfigurationSchema.java ########## @@ -0,0 +1,41 @@ +/* + * 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.configuration.schemas.network; + +import org.apache.ignite.configuration.annotation.Config; +import org.apache.ignite.configuration.annotation.Value; + +/** Node finder configuration. */ +@Config +public class NodeFinderConfigurationSchema { + /** Node finder type. */ + @Value(hasDefault = true) + public final String type = NodeFinderType.STATIC.name(); + + /** Cluster nodes. */ + @Value(hasDefault = true) + public final String[] netClusterNodes = new String[0]; Review comment: what's "cluster nodes"? ########## File path: modules/network-api/src/main/java/org/apache/ignite/network/ClusterLocalConfiguration.java ########## @@ -38,7 +38,10 @@ * @param name Local name. * @param serializationRegistry Message serialization registry. */ - public ClusterLocalConfiguration(String name, MessageSerializationRegistry serializationRegistry) { + public ClusterLocalConfiguration( Review comment: what is this change for? This stuff perfectly fits on one line ########## File path: modules/api/src/main/java/org/apache/ignite/configuration/schemas/network/NetworkConfigurationSchema.java ########## @@ -17,24 +17,28 @@ package org.apache.ignite.configuration.schemas.network; +import org.apache.ignite.configuration.annotation.ConfigValue; import org.apache.ignite.configuration.annotation.ConfigurationRoot; import org.apache.ignite.configuration.annotation.ConfigurationType; -import org.apache.ignite.configuration.annotation.Value; -import org.apache.ignite.configuration.validation.Max; -import org.apache.ignite.configuration.validation.Min; /** * Configuration schema for network endpoint subtree. */ @ConfigurationRoot(rootName = "network", type = ConfigurationType.LOCAL) public class NetworkConfigurationSchema { - /** Network port. */ - @Min(1024) - @Max(0xFFFF) - @Value(hasDefault = true) - public final int port = 47500; + /** Server configuration. */ + @ConfigValue + public final ServerConfigurationSchema server = new ServerConfigurationSchema(); Review comment: Should most of the nested configurations' fields be marked with `@Immutable`? ########## File path: modules/cli/src/integrationTest/java/org/apache/ignite/cli/ITConfigCommandTest.java ########## @@ -65,21 +75,27 @@ private void setup(@TempDir Path workDir) throws IOException { clientPort = getAvailablePort(); networkPort = getAvailablePort(); - String configStr = "network.port=" + networkPort + "\n" + + String configStr = "network.server.port=" + networkPort + "\n" + Review comment: May I suggest a little bit more clean approach: ``` String configStr = String.join("\n", "network.server.port=" + networkPort, "rest.port=" + restPort, "rest.portRange=0", "clientConnector.port=" + clientPort, "clientConnector.portRange=0" ); ``` ########## File path: modules/cli/src/integrationTest/java/org/apache/ignite/cli/ITConfigCommandTest.java ########## @@ -114,13 +130,10 @@ public void setAndGetWithManualHost() { ); assertEquals(0, exitCode); - assertEquals( - "\"{\"clientConnector\":{\"connectTimeout\":5000,\"port\":" + clientPort + ",\"portRange\":0}," + - "\"network\":{\"netClusterNodes\":[],\"port\":" + networkPort + "}," + - "\"node\":{\"metastorageNodes\":[\"localhost1\"]}," + - "\"rest\":{\"port\":" + restPort + ",\"portRange\":0}}\"" + nl, - unescapeQuotes(out.toString()) - ); + + DocumentContext document = JsonPath.parse(removeTrailingQuotes(unescapeQuotes(out.toString()))); Review comment: Cool, I didn't know we had this library as a dependency... ########## File path: modules/network/src/main/java/org/apache/ignite/internal/network/netty/NettyServer.java ########## @@ -260,6 +254,29 @@ public NettyServer( } } + /** + * Try bind this server to the port. + * + * @param port Target port. + * @param endPort Last port that server can be bound to. + * @param fut Future. + */ + private void tryBind(int port, int endPort, CompletableFuture<Channel> fut) { + if (port > endPort) + fut.completeExceptionally(new IllegalStateException("No available port in range")); + + NettyUtils.toChannelCompletableFuture(bootstrap.bind(port)).handle((channel, throwable) -> { + if (throwable != null) { Review comment: redundant parentheses >_< ########## File path: modules/network-api/src/main/java/org/apache/ignite/network/ClusterServiceFactory.java ########## @@ -28,12 +27,10 @@ * * @param context Cluster context. * @param nodeConfiguration Node configuration. - * @param nodeFinderSupplier Supplier that provides node finder for discovering the initial cluster members. * @return New cluster service. */ ClusterService createClusterService( ClusterLocalConfiguration context, - ConfigurationManager nodeConfiguration, - Supplier<NodeFinder> nodeFinderSupplier + ConfigurationManager nodeConfiguration Review comment: I would suggest to replace this parameter with `NetworkView`. That way it would be much easier to mock and it is more descriptive. ########## File path: modules/network/src/integrationTest/java/org/apache/ignite/utils/ClusterServiceTestUtils.java ########## @@ -44,30 +45,32 @@ * * @param nodeName Local name. * @param port Local port. - * @param nodeFinder Node finder for discovering the initial cluster members. + * @param nodeFinderConfigurer Node finder configurer. * @param msgSerializationRegistry Message serialization registry. * @param clusterSvcFactory Cluster service factory. */ public static ClusterService clusterService( String nodeName, int port, - NodeFinder nodeFinder, + Consumer<NodeFinderChange> nodeFinderConfigurer, Review comment: As I said above, I don't like this approach. I would prefer using a `NodeFinder` instance as before. ########## File path: modules/network/src/main/java/org/apache/ignite/network/LocalPortRangeNodeFinder.java ########## @@ -42,6 +45,13 @@ public LocalPortRangeNodeFinder(int startPort, int endPort) { .collect(toUnmodifiableList()); } + public static Consumer<NodeFinderChange> createConfiguration(int startPort, int endPort) { Review comment: Missing javadoc ########## File path: modules/api/src/main/java/org/apache/ignite/configuration/schemas/network/ServerConfigurationSchema.java ########## @@ -0,0 +1,61 @@ +/* + * 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.configuration.schemas.network; + +import org.apache.ignite.configuration.annotation.Config; +import org.apache.ignite.configuration.annotation.Value; +import org.apache.ignite.configuration.validation.Max; +import org.apache.ignite.configuration.validation.Min; + +/** Server socket configuration. See <a href="https://man7.org/linux/man-pages/man7/tcp.7.html";>TCP docs</a>. */ +@Config +public class ServerConfigurationSchema { + /** Network port. */ + @Min(1024) + @Max(0xFFFF) + @Value(hasDefault = true) + public final int port = 47500; + + /** Network port range. */ + @Min(0) + @Value(hasDefault = true) + public final int portRange = 100; Review comment: Are you sure that the default `portRange` should not be 0, at least for test purposes? ########## File path: modules/metastorage-client/src/integrationTest/java/org/apache/ignite/internal/metastorage/client/ITMetaStorageServiceTest.java ########## @@ -202,7 +202,7 @@ public void beforeTest() throws Exception { addr -> ClusterServiceTestUtils.clusterService( addr.toString(), addr.port(), - nodeFinder, + LocalPortRangeNodeFinder.createConfiguration(NODE_PORT_BASE, NODE_PORT_BASE + NODES), Review comment: ok, this looks weird. Can a NodeFinder have a non-static method, called "updateConfiguration", that accepts a `NodeFinderChange` instead? ########## File path: modules/network/src/main/java/org/apache/ignite/internal/network/netty/NettyServer.java ########## @@ -260,6 +254,29 @@ public NettyServer( } } + /** + * Try bind this server to the port. + * + * @param port Target port. + * @param endPort Last port that server can be bound to. + * @param fut Future. + */ + private void tryBind(int port, int endPort, CompletableFuture<Channel> fut) { + if (port > endPort) + fut.completeExceptionally(new IllegalStateException("No available port in range")); + + NettyUtils.toChannelCompletableFuture(bootstrap.bind(port)).handle((channel, throwable) -> { Review comment: I would suggest using the `bootstrap.bind(port).addListener` method instead, because otherwise you are creating rather pointless intermediate Future instances. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
