Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Cluster-bus port configurable with new cluster-port config #9389

Merged
merged 8 commits into from
Oct 19, 2021

Conversation

hwware
Copy link
Collaborator

@hwware hwware commented Aug 18, 2021

Background:
Currently, for a cluster node, cluster bus port = command port+ 10000, which is a fixed value.

We would like to make the value flexible, thus user could have more choice for the offset between command port
and cluster bus port.

The parameter "cluster-port" can be added in redis.conf, the value could be from 1 to 65535.

Example for 4 node config files in a clsuter:

node1:

port 6381
cluster-enabled yes
cluster-config-file nodes1-6381.conf
cluster-node-timeout 15000
cluster-port 10000

node2:

port 6382
cluster-enabled yes
cluster-config-file nodes2-6382.conf
cluster-node-timeout 15000
cluster-port 10001

node3:

port 6383
cluster-enabled yes
cluster-config-file nodes3-6383.conf
cluster-node-timeout 15000
cluster-port 20000

node4:

port 6384
cluster-enabled yes
cluster-config-file nodes4-6384.conf
cluster-node-timeout 15000

the result:

image

@hwware hwware requested review from oranagra and yossigo August 18, 2021 15:21
redis.conf Outdated Show resolved Hide resolved
@hwware hwware changed the title Make cluster-port-incr variable configurable, the default value is 10000 Add cluster-port parameter in the redis.conf Sep 23, 2021
@hwware
Copy link
Collaborator Author

hwware commented Sep 23, 2021

@madolson I update the codes for the requirement: make the cluster port more flexible, instead of using a fixed value: command port+ 10000. Please take a look, Thanks

Copy link
Contributor

@madolson madolson left a comment

Choose a reason for hiding this comment

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

Implementation is good, just some style comments

redis.conf Outdated Show resolved Hide resolved
src/cluster.c Outdated Show resolved Hide resolved
src/cluster.h Outdated Show resolved Hide resolved
src/server.h Outdated Show resolved Hide resolved
src/cluster.c Outdated Show resolved Hide resolved
redis.conf Outdated Show resolved Hide resolved
Copy link
Contributor

@madolson madolson left a comment

Choose a reason for hiding this comment

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

I suppose we should update the https://redis.io/commands/cluster-meet documentation in parallel. I realized that it was wrong.

@madolson
Copy link
Contributor

@redis/core-team Approval for the new config name

@madolson madolson added approval-needed Waiting for core team approval to be merged state:major-decision Requires core team consensus labels Sep 30, 2021
@hwware
Copy link
Collaborator Author

hwware commented Sep 30, 2021

I suppose we should update the https://redis.io/commands/cluster-meet documentation in parallel. I realized that it was wrong.

I will update the documentation with more details after this PR is merged. Thanks for your words.

redis.conf Outdated Show resolved Hide resolved
Copy link
Contributor

@madolson madolson left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

LGTM. maybe we wanna add some basic test coverage?

@hwware hwware force-pushed the cluster_port_incr_config branch from b8f5cd8 to 6cc7bea Compare October 12, 2021 21:25
@madolson madolson removed the approval-needed Waiting for core team approval to be merged label Oct 18, 2021
@madolson madolson changed the title Add cluster-port parameter in the redis.conf Make Cluster-bus port configurable with new cluster-port config Oct 18, 2021
@madolson madolson merged commit 1c2b5f5 into redis:unstable Oct 19, 2021
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Oct 19, 2021
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Feb 25, 2022
In redis#9389, we add a new cluster-port config and make cluster bus port
configurable. Now add support for this parameter in redis-cli.

Mainly for `--cluster create` and `--cluster add-node`, we can use
`ip:port@bus_port` to pass the `cluster-port` parameter. The bus_port
is optional, if not specified it is the same as before.

Also add the `bus_port` field in `--cluster backup`, we also need
to backup the `cluster-port`.

Fixes redis#10342
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Feb 27, 2022
In `CLUSTER MEET`, bus-port argument was added in 11436b1.
For cluster announce ip / port implementation, part of the
4.0-RC1.

And in redis#9389, we add a new cluster-port config and make
cluster bus port configurable, part of the 7.0-RC1.
oranagra pushed a commit that referenced this pull request Jun 23, 2022
In `CLUSTER MEET`, bus-port argument was added in 11436b1.
For cluster announce ip / port implementation, part of the
4.0-RC1.

And in #9389, we add a new cluster-port config and make
cluster bus port configurable, part of the 7.0-RC1.
oranagra pushed a commit that referenced this pull request Jul 11, 2022
In #9389, we add a new `cluster-port` config and make cluster bus port configurable,
and currently redis-cli --cluster create/add-node doesn't support with a configurable `cluster-port` instance.
Because redis-cli uses the old way (port + 10000) to send the `CLUSTER MEET` command.

Now we add this support on redis-cli `--cluster`, note we don't need to explicitly pass in the
`cluster-port` parameter, we can get the real `cluster-port` of the node in `clusterManagerNodeLoadInfo`,
so the `--cluster create` and `--cluster add-node` interfaces have not changed.

We will use the `cluster-port` when we are doing `CLUSTER MEET`, also note that `CLUSTER MEET` bus-port
parameter was added in 4.0, so if the bus_port (the one in redis-cli) is 0, or equal (port + 10000),
we just call `CLUSTER MEET` with 2 arguments, using the old form.

Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
In `CLUSTER MEET`, bus-port argument was added in 11436b1.
For cluster announce ip / port implementation, part of the
4.0-RC1.

And in redis#9389, we add a new cluster-port config and make
cluster bus port configurable, part of the 7.0-RC1.
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
In redis#9389, we add a new `cluster-port` config and make cluster bus port configurable,
and currently redis-cli --cluster create/add-node doesn't support with a configurable `cluster-port` instance.
Because redis-cli uses the old way (port + 10000) to send the `CLUSTER MEET` command.

Now we add this support on redis-cli `--cluster`, note we don't need to explicitly pass in the
`cluster-port` parameter, we can get the real `cluster-port` of the node in `clusterManagerNodeLoadInfo`,
so the `--cluster create` and `--cluster add-node` interfaces have not changed.

We will use the `cluster-port` when we are doing `CLUSTER MEET`, also note that `CLUSTER MEET` bus-port
parameter was added in 4.0, so if the bus_port (the one in redis-cli) is 0, or equal (port + 10000),
we just call `CLUSTER MEET` with 2 arguments, using the old form.

Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants