XComp commented on code in PR #21691:
URL: https://github.com/apache/flink/pull/21691#discussion_r1073310290
##########
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/netty/NettyPartitionRequestClientTest.java:
##########
@@ -55,11 +54,12 @@
/** Tests for {@link NettyPartitionRequestClient}. */
@RunWith(Parameterized.class)
public class NettyPartitionRequestClientTest {
- @Parameterized.Parameter public boolean connectionReuseEnabled;
+ @Parameterized.Parameter
+ public boolean connectionReuseEnabled;
@Parameterized.Parameters(name = "connection reuse enabled = {0}")
public static Object[] parameters() {
- return new Object[][] {new Object[] {true}, new Object[] {false}};
+ return new Object[][]{new Object[]{true}, new Object[]{false}};
Review Comment:
Why did this formatting change?
##########
flink-clients/src/test/java/org/apache/flink/client/program/ClientTest.java:
##########
@@ -88,8 +86,6 @@ class ClientTest {
private Plan plan;
- private NetUtils.Port port;
Review Comment:
I verified that we don't need this port for MiniCluster usages. The
MiniCluster's JM port is set to 0 during startup (see
[MiniClusterResource:214](https://github.com/apache/flink/blob/74569dc137038dfdb27b4216ee79cf36bbd789c9/flink-runtime/src/test/java/org/apache/flink/runtime/testutils/MiniClusterResource.java#L214)).
:+1:
##########
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/netty/NettyPartitionRequestClientTest.java:
##########
@@ -307,6 +304,7 @@ private NettyPartitionRequestClient
createPartitionRequestClient(
*
* @param channel the channel to execute tasks for
* @param deadline maximum timestamp in ms to stop waiting further
+ *
Review Comment:
not necessary
##########
flink-runtime/src/test/java/org/apache/flink/runtime/taskexecutor/TaskExecutorTest.java:
##########
@@ -2244,10 +2243,7 @@ public void testDisconnectFromJobMasterWhenNewLeader()
throws Exception {
@Test(timeout = 10000L)
public void testLogNotFoundHandling() throws Throwable {
- try (NetUtils.Port port = NetUtils.getAvailablePort()) {
- int dataPort = port.getPort();
-
- configuration.setInteger(NettyShuffleEnvironmentOptions.DATA_PORT,
dataPort);
+ configuration.setInteger(NettyShuffleEnvironmentOptions.DATA_PORT,
0);
Review Comment:
I verified that setting the port to 0 is good enough. It's used in
[TaskSubmissionTestEnvironment:290](https://github.com/apache/flink/blob/324b54cf413229f78e6e709aaaa2e16a03d45df1/flink-runtime/src/test/java/org/apache/flink/runtime/taskexecutor/TaskSubmissionTestEnvironment.java#L290).
##########
flink-tests/src/test/java/org/apache/flink/test/runtime/IPv6HostnamesITCase.java:
##########
@@ -146,15 +145,13 @@ private Inet6Address getLocalIPv6Address() {
// test whether Akka's netty can bind to the
address
log.info("Testing whether Akka can use " + addr);
- try (NetUtils.Port port =
NetUtils.getAvailablePort()) {
- final RpcService rpcService =
- RpcSystem.load()
- .localServiceBuilder(new
Configuration())
-
.withBindAddress(addr.getHostAddress())
- .withBindPort(port.getPort())
- .createAndStart();
- rpcService.closeAsync().get();
- }
+ final RpcService rpcService =
+ RpcSystem.load()
+ .localServiceBuilder(new
Configuration())
+
.withBindAddress(addr.getHostAddress())
+ .withBindPort(0)
Review Comment:
This change doesn't seem to have any impact on the executed code (in both
cases, the port doesn't seem to be set in
[AkkaRpcService:138ff](https://github.com/apache/flink/blob/e9f3ec93aad7cec795c765c937ee71807f5478cf/flink-rpc/flink-rpc-akka/src/main/java/org/apache/flink/runtime/rpc/akka/AkkaRpcService.java#L138).
I'm a bit surprised because I would have expected a port being specified in
both cases after startup of the `AkkaRpcService`. Why is it ok to have no port
set in this case? ...independently of what the test does (immediately closing
the RpcService again). :thinking: Can you shine some light here, Chesnay?
--
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]
