-
Notifications
You must be signed in to change notification settings - Fork 334
Adapt local-cluster.sh
and Dockerfile for bind.listeners
config option
#717
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
Adapt local-cluster.sh
and Dockerfile for bind.listeners
config option
#717
Conversation
- Remove server.yaml and introduce common.yaml config file - Introduce dedicated config files for coordinator and tablet-server to support different config options (host, port) for bind.listeners and Kafka - Adapt config.sh to support common.yaml but ensuring backward compatibility for old server.yaml - Adapt assemblies for newly introduced config files
cc235ae
to
192465f
Compare
- Load new common.yaml from config dir - Allow to specify dedicated configuration files to be loaded in the addition to common.yaml from the config dir - Ensure backward compatibility for old server.yaml - Add server type argument to ConfigurationParserUtils load method and adapt test cases
ac95e20
to
e7dd2f3
Compare
e7dd2f3
to
d8a12fa
Compare
2db8ed8
to
78f51a4
Compare
a355555
to
2dd3ad0
Compare
# Common default configuration file that is used by ALL start scripts, the Fluss Servers and the Lakehouse CLI. | ||
# In addition, there exist additional, dedicated default configuration files for some components that are also placed | ||
# in this folder. The dedicated configuration files are ONLY read by the respective Java applications but NOT by the | ||
# start scripts. In that case, configuration options in the dedicated files overwrite configuration options | ||
# in the common (i.e., this) file. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for reviewer: I suggest to rename server.yaml
to common.yaml
from 0.7.0 onward for the following reasons.
- The file name
server
is a bit misleading, because the config file is not only read by the server but also byconfig.sh
and the lakehouse module. And it's easier to change it now than at some later point. - It avoids confusion. With renaming it, there is a clear separation and it is clear which config files are read. It is either
server.yaml
orcommon.yaml
+ depending on the module, an additional file ([coordinator-server|tablet-server].yaml
).
We can remove server.yaml
when we deprecate the old [coordinator|tablet-server]-host
config options.
Of course, I am open to discuss if renaming the file is a good approach or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, @michaelkoepf, but from the user's perspective, the server.yaml
file is intended for configuring servers, including both the coordinator and tablet servers. This is because it is used when running the coordinator-server.sh
and tablet-server.sh
scripts. On the other hand, config.sh
is an internal shell script that is utilized by coordinator-server.sh
and tablet-server.sh
to manage configurations.
Additionally, in the new lakehouse architecture, the lakehouse-specific script will be removed, and the Flink script will be used to start the tiering service. At that point, the server.yaml
file will be exclusively used for configuring Fluss servers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wuchong is just the renaming of server.yaml
to common.yaml
an issue? or also the additional [coordinator-server|tablet-server].yaml
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelkoepf In my opinion, there is no need to separate the configuration files. Whether starting a Coordinator or a TabletServer, users can simply use the server.yaml
file. If a user wants to run multiple servers on the same machine, they can create multiple YAML files and specify the appropriate YAML file as a parameter for the startup script. This approach is similar to how Kafka handles configurations with the command:
bin/kafka-server-start.sh config/server.properties
This method offers greater flexibility compared to hardcoding 2 YAML files, as multiple TabletServers often require different node IDs and host configurations, making it impractical to share the same yaml
file. I believe supporting startup with a custom YAML file is a missing feature in our current implementation. We should consider creating a separate issue to address this functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a user wants to run multiple servers on the same machine, they can create multiple YAML files and specify the appropriate YAML file as a parameter for the startup script.
this means from 0.7.0 onwards, users must specify their own yaml file when using local-cluster.sh
when they want to use the new bind.listeners
, right? and local-cluster.sh
must accept two additional arguments for the 2 yaml files (one for the tablet and one for the coordinator server)?
I believe supporting startup with a custom YAML file is a missing feature in our current implementation.
I think the functionality in the Java code is already there, right? See here and it should work with coordinator-server.sh
and tablet-server.sh
. We just need to find a solution for local-cluster.sh
(see previous paragraph).
We should consider creating a separate issue to address this functionality.
Separate issue or also separate PR? so i know what i should revert in this PR and what not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this means from 0.7.0 onwards, users must specify their own yaml file when using local-cluster.sh when they want to use the new bind.listeners, right? and local-cluster.sh must accept two additional arguments for the 2 yaml files (one for the tablet and one for the coordinator server)?
We need to adapt local-cluster.sh
to keep the original behavior. I think we can change the line "${FLUSS_BIN_DIR}"/tablet-server.sh start
in local-cluster.sh
to "${FLUSS_BIN_DIR}"/tablet-server.sh start -Dbind.listeners=FLUSS://localhost:0
. So that, Coordinator use the bind.listeners
configured in server.yaml
(i.e., FLUSS://localhost:9123
), and TabletServer use a random port on the same host.
I think the functionality in the Java code is already there, right? See here and it should work with coordinator-server.sh and tablet-server.sh. We just need to find a solution for local-cluster.sh (see previous paragraph).
Cool. I think we need this feature to update the page https://alibaba.github.io/fluss-docs/docs/install-deploy/deploying-distributed-cluster/ that starts Coordinator and TabletServer on the same node.
Separate issue or also separate PR? so i know what i should revert in this PR and what not.
Let's separate into multiple issues and PRs, they are not related too much, this can also make reviewing easier.
af7d889
to
594d6e0
Compare
…].host option in docker images
594d6e0
to
dc54b45
Compare
bind.listeners
configuration option
bind.listeners
configuration optionlocal-cluster.sh
start script for new bind.listeners
configuration option
local-cluster.sh
start script for new bind.listeners
configuration optionbind.listeners
config option
bind.listeners
config optionlocal-cluster.sh
and Dockerfile for bind.listeners
config option
closing this PR. starting from scratch is easier and less confusing than reverting commits. |
Purpose
Linked issue: close #618
Brief change log
Modules
fluss-dist
Choose random port for
bind.listeners
inlocal-cluster.sh
Docker
Ensure backward compatability by removing
bind.listeners
from YAML config files.Tests
n/a
API and Format
n/a
Documentation
n/a