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

Add multiple endpoint listener for fluss server. #531

Merged
merged 3 commits into from
Mar 21, 2025

Conversation

loserwang1024
Copy link
Collaborator

@loserwang1024 loserwang1024 commented Mar 5, 2025

Purpose

Linked issue: close #421 , add multiple endpoint listener for fluss server.

Brief change log

Deprecated config:

  • coordinator.host
  • coordinator.port
  • tablet-server.host
  • tablet-server.port

Added config

  • bind.listeners: required, The network address and port the server binds to for accepting connections.
  • advertised.listeners: required, The externally advertised address and port for client connections.
  • internal.listener.name: required, and must be included in coordinator.listener and tablet-server.listeners. Otherwise, internal servers can not communicate.

Tests

API and Format

Documentation

@loserwang1024
Copy link
Collaborator Author

@loserwang1024 loserwang1024 force-pushed the add-multi-endpoint-tmp branch 5 times, most recently from 3390a81 to 3321cac Compare March 13, 2025 14:26
@loserwang1024 loserwang1024 force-pushed the add-multi-endpoint-tmp branch from 3321cac to a1662d1 Compare March 13, 2025 14:59
@loserwang1024
Copy link
Collaborator Author

@wuchong , rebase and refactor this pr, CC

Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

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

  1. Please update server.yaml as well.
  2. Please create an issue to update local_cluster.sh to adapt the config changes.
  3. Please create an issue to update all the documentation.
  4. Validate the configs, e.g, all listener names are not duplicated, ports are not duplicated. All the listener names in advertised.listeners and internal.listener.name can be found in bind.listeners? Take Kafka kafka.utils.CoreUtils.listenerListToEndPoints as an example, and add tests.

pbCoordinatorServer.getNodeId(),
pbCoordinatorServer.getHost(),
pbCoordinatorServer.getPort(),
Endpoint.parseEndpoints(pbCoordinatorServer.getListeners()),
Copy link
Member

Choose a reason for hiding this comment

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

to support compatible with old version (host & prot).
and add UT for the compatibility test (similar to com.alibaba.fluss.server.coordinator.TableManagerITCase#testMetadata)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add com.alibaba.fluss.server.coordinator.TableManagerITCase#testMetadataCompatibility

@loserwang1024 loserwang1024 force-pushed the add-multi-endpoint-tmp branch 2 times, most recently from 349a666 to 6a37208 Compare March 17, 2025 09:59
@loserwang1024 loserwang1024 force-pushed the add-multi-endpoint-tmp branch from 6a37208 to 6b61950 Compare March 17, 2025 11:42
@loserwang1024
Copy link
Collaborator Author

  1. Add subtask to modify shell and docs
  2. modify this pr based on cr.
    @wuchong , CC

@loserwang1024 loserwang1024 requested a review from wuchong March 17, 2025 12:51
package com.alibaba.fluss.exception;

/** Exception thrown when the endpoint is not available. */
public class EndpointNotAvailableException extends ApiException {
Copy link
Member

Choose a reason for hiding this comment

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

This is only used in the server, so doesn't need to be an ApiException. Move it to com.alibaba.fluss.server.exception. Besides, ApiException strips the exception stack, which is very inconvenient to debug. Here it should extend RuntimeException.

return Objects.hash(host, port, listenerName);
}

public String connectionString() {
Copy link
Member

Choose a reason for hiding this comment

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

listenerString to keep align with toListenersString, fromListenersString.

* limitations under the License.
*/

package cluster;
Copy link
Member

Choose a reason for hiding this comment

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

wrong package

@@ -621,8 +670,24 @@ public Builder setClusterConf(Configuration clusterConf) {
return this;
}

/** Sets the listeners of tablet servers. */
public Builder setTabletServerListeners(String tabletServerListeners) {
Copy link
Member

Choose a reason for hiding this comment

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

Good refactor!

required int32 port = 3;
// host and port is used for client.
optional string host = 2;
optional int32 port = 3;
Copy link
Member

Choose a reason for hiding this comment

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

we can't change required to optional as it breaks compatibility, new version server sent events to the old version server, the events will be failed to deserialize on the old version server.

@wuchong
Copy link
Member

wuchong commented Mar 20, 2025

I pushed a commit to fix the remaining minor comments.

@wuchong
Copy link
Member

wuchong commented Mar 21, 2025

@loserwang1024 I'm going to merge it. If there are any problems, we can fix in the following PRs.

@wuchong wuchong merged commit 157d252 into alibaba:main Mar 21, 2025
4 of 5 checks passed
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.

Fluss support multiple advertised listeners
2 participants