sashapolo commented on code in PR #3297:
URL: https://github.com/apache/ignite-3/pull/3297#discussion_r1505791559
##########
modules/network/src/test/java/org/apache/ignite/internal/network/netty/NettyServerTest.java:
##########
@@ -140,6 +146,22 @@ public void testStartTwice() {
assertThrows(IgniteInternalException.class, server::start);
}
+ /**
+ * Tests that bootstrap tries to bind to host specified in
network.bindHost.
+ */
+ @Test
+ public void testBindHost() {
Review Comment:
Is it possible to test a happy-case scenario?
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/configuration/ClientConnectorConfigurationSchema.java:
##########
@@ -36,6 +36,10 @@ public class ClientConnectorConfigurationSchema {
@Value(hasDefault = true)
public final int port = 10800;
+ /** Address (IP or hostname) to listen to. */
Review Comment:
What does the default value mean? It should be specified in the javadoc
##########
modules/network/src/main/java/org/apache/ignite/internal/network/configuration/NetworkConfigurationSchema.java:
##########
@@ -36,6 +36,10 @@ public class NetworkConfigurationSchema {
@Value(hasDefault = true)
public final int port = DEFAULT_PORT;
+ /** Address (IP or hostname) to listen to. */
Review Comment:
Same issue
##########
modules/client-handler/src/integrationTest/java/org/apache/ignite/client/handler/ItClientHandlerTest.java:
##########
@@ -459,6 +461,17 @@ void testHandshakeInvalidVersionReturnsError() throws
Exception {
}
}
+ @Test
+ void connectWithHostSpecified(
Review Comment:
Can we test a happy-case scenario?
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientHandlerModule.java:
##########
@@ -334,6 +340,15 @@ protected void initChannel(Channel ch) {
return;
}
+ if (bindFut.cause() instanceof UnresolvedAddressException) {
+ result.completeExceptionally(
+ new IgniteException(
+ INTERNAL_ERR,
+ "Failed to start thin connector endpoint,
address \"" + host + "\" is not found"
+ )
+ );
Review Comment:
I guess you are missing a `return` here. But I would actually prefer to
refactor this code to look like this:
```
if (bind.isSuccess()) {
...
} else if (bindFut.cause() instanceof BindException) {
...
} else if (bindFut.cause() instanceof UnresolvedAddressException) {
...
} else {
...
}
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/NettyServer.java:
##########
@@ -138,16 +138,24 @@ public void initChannel(SocketChannel ch) {
});
int port = configuration.port();
+ String host = configuration.listenAddress();
var bindFuture = new CompletableFuture<Channel>();
- bootstrap.bind(port).addListener((ChannelFuture future) -> {
+ ChannelFuture channelFuture = host.isEmpty() ?
bootstrap.bind(port) : bootstrap.bind(host, port);
+
+ channelFuture.addListener((ChannelFuture future) -> {
if (future.isSuccess()) {
bindFuture.complete(future.channel());
} else if (future.isCancelled()) {
bindFuture.cancel(true);
} else {
- bindFuture.completeExceptionally(new
IllegalStateException("Port " + port + " is not available."));
+ bindFuture.completeExceptionally(
+ new IllegalStateException(host.isEmpty()
+ ? "Port " + port + " is not available."
+ : String.format("Host %s:%d is not
available", host, port)
Review Comment:
`Host` -> `Address` maybe?
##########
modules/client-handler/src/integrationTest/java/org/apache/ignite/client/handler/TestServer.java:
##########
@@ -84,6 +90,9 @@ public TestServer() {
generator,
new TestConfigurationValidator()
);
+ this.connectorConfiguration = connectorConfiguration == null
+ ?
configurationManager.configurationRegistry().getConfiguration(ClientConnectorConfiguration.KEY)
Review Comment:
Do we really need this? Can we always pass a non-null configuration and get
rid of the `configurationManager`?
--
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]
