Skip to content

Conversation

@frankgh
Copy link
Contributor

@frankgh frankgh commented Aug 3, 2023

Currently, INodeProvisionStrategy supports two strategies OneNetworkInterface and MultipleNetworkInterfaces. However the seedPort, storagePorts, nativeTransportPorts, and jmxPorts are always fixed or a function of the node number.

In order to better support parallel test runs, this commit introduces support for dynamic port allocation for the seedPort, storagePorts, nativeTransportPorts, and jmxPorts.

When enabled, the port allocation will be dynamic, an available port for the given bind address will be used instead of the previously statically allocated port number. This would allow us to run multiple clusters within the same test, or it will enable us to run in-jvm dtests in parallel given that the tests do not have other inter-test dependencies.

A new option in the cluster builder is introduced .withDynamicPortAllocation(boolean). To enable the new feature one must request dynamic port allocation while building the cluster.

CASSANDRA-18722

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method was unused

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another option is to provide a new interface PortAllocationStrategy and we'd have default implementations as well as the DynamicPortAllocationStrategy. I didn't go with this approach because I don't foresee a different port allocation strategy other than the default and dynamic in the near future.

Copy link
Member

@dineshjoshi dineshjoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of checking for nodeNum == 1 in storagePort, you could just define default methods on the interface for seedIP() and seedPort(), just calling ipAddress(1) and storagePort(1) respectively and reduce the amount of methods and make the portMap more regular with entries for storagePort for all.

 "nativeTransportPort:2" -> {Integer@17131} 63721
 "nativeTransportPort:1" -> {Integer@17107} 63719
 "jmxPort:2" -> {Integer@17133} 63722
 "jmxPort:1" -> {Integer@17109} 63720
 "storagePort:2" -> {Integer@16549} 63709
 "seedPort" -> {Integer@16550} 63708```

…ework

Currently, `INodeProvisionStrategy` supports two strategies `OneNetworkInterface` and
`MultipleNetworkInterfaces`. However the `seedPort`, `storagePorts`, `nativeTransportPorts`,
and `jmxPorts` are always fixed or a function of the node number.

In order to better support parallel test runs, this commit introduces support for dynamic
port allocation for the `seedPort`, `storagePorts`, `nativeTransportPorts`, and `jmxPorts`.

When enabled, the port allocation will be dynamic, an available port for the given bind
address will be used instead of the previously statically allocated port number. This would
allow us to run multiple clusters within the same test, or it will enable us to run in-jvm
dtests in parallel given that the tests do not have other inter-test dependencies.

A new option in the cluster builder is introduced `.withDynamicPortAllocation(boolean)`. To
enable the new feature one must request dynamic port allocation while building the cluster.

[CASSANDRA-18722](https://issues.apache.org/jira/browse/CASSANDRA-18722)
Copy link
Contributor

@yifan-c yifan-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one nit.
+1 on the patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This key does not contain ":", while all the others contain ":".
Are the keys going to be printed. If so, we can go a bit for verbose to have node1 instead of 1 and probably @ suits better than :.

@frankgh
Copy link
Contributor Author

frankgh commented Aug 28, 2023

Closed via 6ffa43f

@frankgh frankgh closed this Aug 28, 2023
@frankgh frankgh deleted the CASSANDRA-18722 branch August 28, 2023 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants