You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2022/02/28 19:53:30 UTC
[GitHub] [activemq-artemis] jbertram opened a new pull request #3969: ARTEMIS-3699 expose ephemeral port on NettyAcceptor
jbertram opened a new pull request #3969:
URL: https://github.com/apache/activemq-artemis/pull/3969
It sometimes makes sense to set an acceptor's port to 0 to allow the JVM
to select an ephemeral port (e.g. in embedded integration tests). This
commit adds a new getter on NettyAcceptor so tests can programmtically
determine the ephemeral port used by the acceptor.
This commit also changes the ACCEPTOR_STARTED notification and the
related logging to clarify the actual port value where clients can
connect.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3969: ARTEMIS-3699 expose actual port on NettyAcceptor
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3969:
URL: https://github.com/apache/activemq-artemis/pull/3969#discussion_r818583172
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/remoting/Acceptor.java
##########
@@ -83,4 +83,15 @@
default ProtocolHandler getProtocolHandler() {
return null;
}
+
+ /**
+ * This is a utility method for Socket-based acceptor implementations to get the actual port used.
+ * This is useful for configurations which specify a port number of 0 which allows the JVM to select
+ * an ephemeral port.
+ *
+ * @return the actual port used if using a Socket-based acceptor implementation; null otherwise
+ */
+ default Integer getActualPort() {
+ return null;
Review comment:
Perhaps just -1 rather than a null? Keeps it all-int, avoiding need for Integer boxing the actual impl doesnt need and potential for NPE, while still being equally invalid as a port number?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3969: ARTEMIS-3699 expose actual port on NettyAcceptor
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3969:
URL: https://github.com/apache/activemq-artemis/pull/3969
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a change in pull request #3969: ARTEMIS-3699 expose actual port on NettyAcceptor
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3969:
URL: https://github.com/apache/activemq-artemis/pull/3969#discussion_r817992561
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/remoting/impl/netty/NettyAcceptorTest.java
##########
@@ -123,4 +124,20 @@ public void testAutoStart() throws Exception {
assertTrue(server.getRemotingService().getAcceptor("start").isStarted());
assertFalse(server.getRemotingService().getAcceptor("noStart").isStarted());
}
+
+ @Test
+ public void testActualPort() throws Exception {
+ String acceptor1Name = RandomUtil.randomString();
+ String acceptor2Name = RandomUtil.randomString();
+ String acceptor3Name = RandomUtil.randomString();
+ ActiveMQServer server = createServer(false, createDefaultInVMConfig());
+ server.getConfiguration().addAcceptorConfiguration(acceptor1Name, "tcp://127.0.0.1:0");
+ server.getConfiguration().addAcceptorConfiguration(acceptor2Name, "tcp://127.0.0.1:0");
+ server.getConfiguration().addAcceptorConfiguration(acceptor3Name, "tcp://127.0.0.1:61616");
+ server.start();
+ assertTrue(((NettyAcceptor)server.getRemotingService().getAcceptor(acceptor1Name)).getActualPort() != 0);
Review comment:
Fair enough. Change made.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3969: ARTEMIS-3699 expose actual port on NettyAcceptor
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3969:
URL: https://github.com/apache/activemq-artemis/pull/3969#discussion_r817935136
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/remoting/impl/netty/NettyAcceptorTest.java
##########
@@ -123,4 +124,20 @@ public void testAutoStart() throws Exception {
assertTrue(server.getRemotingService().getAcceptor("start").isStarted());
assertFalse(server.getRemotingService().getAcceptor("noStart").isStarted());
}
+
+ @Test
+ public void testActualPort() throws Exception {
+ String acceptor1Name = RandomUtil.randomString();
+ String acceptor2Name = RandomUtil.randomString();
+ String acceptor3Name = RandomUtil.randomString();
+ ActiveMQServer server = createServer(false, createDefaultInVMConfig());
+ server.getConfiguration().addAcceptorConfiguration(acceptor1Name, "tcp://127.0.0.1:0");
+ server.getConfiguration().addAcceptorConfiguration(acceptor2Name, "tcp://127.0.0.1:0");
+ server.getConfiguration().addAcceptorConfiguration(acceptor3Name, "tcp://127.0.0.1:61616");
+ server.start();
Review comment:
Are you sure the activation here is not asynchronous? meaning you should use a Wait.assertTrue... or perhaps a Wait.assertTrue(server::isActive) before you check the next assertions?
just trying to avoid an intermittent test failure here due to the async start.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3969: ARTEMIS-3699 expose actual port on NettyAcceptor
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3969:
URL: https://github.com/apache/activemq-artemis/pull/3969#discussion_r817968168
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/remoting/impl/netty/NettyAcceptorTest.java
##########
@@ -123,4 +124,20 @@ public void testAutoStart() throws Exception {
assertTrue(server.getRemotingService().getAcceptor("start").isStarted());
assertFalse(server.getRemotingService().getAcceptor("noStart").isStarted());
}
+
+ @Test
+ public void testActualPort() throws Exception {
+ String acceptor1Name = RandomUtil.randomString();
+ String acceptor2Name = RandomUtil.randomString();
+ String acceptor3Name = RandomUtil.randomString();
+ ActiveMQServer server = createServer(false, createDefaultInVMConfig());
+ server.getConfiguration().addAcceptorConfiguration(acceptor1Name, "tcp://127.0.0.1:0");
+ server.getConfiguration().addAcceptorConfiguration(acceptor2Name, "tcp://127.0.0.1:0");
+ server.getConfiguration().addAcceptorConfiguration(acceptor3Name, "tcp://127.0.0.1:61616");
+ server.start();
Review comment:
Yep I think it will be (thats what prompted my comment about the volatile) so that is a good point.
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/remoting/impl/netty/NettyAcceptorTest.java
##########
@@ -123,4 +124,20 @@ public void testAutoStart() throws Exception {
assertTrue(server.getRemotingService().getAcceptor("start").isStarted());
assertFalse(server.getRemotingService().getAcceptor("noStart").isStarted());
}
+
+ @Test
+ public void testActualPort() throws Exception {
+ String acceptor1Name = RandomUtil.randomString();
+ String acceptor2Name = RandomUtil.randomString();
+ String acceptor3Name = RandomUtil.randomString();
+ ActiveMQServer server = createServer(false, createDefaultInVMConfig());
+ server.getConfiguration().addAcceptorConfiguration(acceptor1Name, "tcp://127.0.0.1:0");
+ server.getConfiguration().addAcceptorConfiguration(acceptor2Name, "tcp://127.0.0.1:0");
+ server.getConfiguration().addAcceptorConfiguration(acceptor3Name, "tcp://127.0.0.1:61616");
+ server.start();
+ assertTrue(((NettyAcceptor)server.getRemotingService().getAcceptor(acceptor1Name)).getActualPort() != 0);
Review comment:
Having to get the remoting service from the server, then the acceptor from that, then cast the acceptor to NettyAcceptor to then use this is still all a bit ugly. Even getting rid of the last bit if nothing else would be a decent improvement. The 'invm' acceptor seems to have an 'id' concept, could it return that for this and let the interface have a method?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a change in pull request #3969: ARTEMIS-3699 expose actual port on NettyAcceptor
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3969:
URL: https://github.com/apache/activemq-artemis/pull/3969#discussion_r818953625
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/remoting/Acceptor.java
##########
@@ -83,4 +83,15 @@
default ProtocolHandler getProtocolHandler() {
return null;
}
+
+ /**
+ * This is a utility method for Socket-based acceptor implementations to get the actual port used.
+ * This is useful for configurations which specify a port number of 0 which allows the JVM to select
+ * an ephemeral port.
+ *
+ * @return the actual port used if using a Socket-based acceptor implementation; null otherwise
+ */
+ default Integer getActualPort() {
+ return null;
Review comment:
Done.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a change in pull request #3969: ARTEMIS-3699 expose actual port on NettyAcceptor
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3969:
URL: https://github.com/apache/activemq-artemis/pull/3969#discussion_r817992288
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/remoting/impl/netty/NettyAcceptorTest.java
##########
@@ -123,4 +124,20 @@ public void testAutoStart() throws Exception {
assertTrue(server.getRemotingService().getAcceptor("start").isStarted());
assertFalse(server.getRemotingService().getAcceptor("noStart").isStarted());
}
+
+ @Test
+ public void testActualPort() throws Exception {
+ String acceptor1Name = RandomUtil.randomString();
+ String acceptor2Name = RandomUtil.randomString();
+ String acceptor3Name = RandomUtil.randomString();
+ ActiveMQServer server = createServer(false, createDefaultInVMConfig());
+ server.getConfiguration().addAcceptorConfiguration(acceptor1Name, "tcp://127.0.0.1:0");
+ server.getConfiguration().addAcceptorConfiguration(acceptor2Name, "tcp://127.0.0.1:0");
+ server.getConfiguration().addAcceptorConfiguration(acceptor3Name, "tcp://127.0.0.1:61616");
+ server.start();
Review comment:
Good point. Change made.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3969: ARTEMIS-3699 expose ephemeral port on NettyAcceptor
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3969:
URL: https://github.com/apache/activemq-artemis/pull/3969#discussion_r816627195
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -715,6 +717,9 @@ private void startServerChannels() {
Channel serverChannel = null;
try {
serverChannel = bootstrap.bind(address).syncUninterruptibly().channel();
+ if (port == 0 && serverChannel.localAddress() instanceof InetSocketAddress) {
+ ephemeralPort = ((InetSocketAddress)serverChannel.localAddress()).getPort();
+ }
Review comment:
I'd suggest going with 'actualPort' and setting it in each case (port== 0 or not), thus having the getter always return the port value, whether it is a specified non-0 port or a port-0 selected-on-bind value.
That way the method is always useful, even in case of a configured port to get the value (as opposed to trying the config retrieval sequence from the JIRA), and it wont require you to essentially already know in advance of calling whether the acceptor had one configured or selected on bind.
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -241,6 +241,8 @@
private volatile Object providerAgnosticSslContext;
+ private int ephemeralPort = 0;
Review comment:
This should probably be volatile for safety since it seems it will have to be accessed by a different thread in actual use (most likely straight after setting), so unless there is an explicit barrier between them somewhere it would be unreliable otherwise.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org