-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Support configurable ExchangeManager #26629
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
Support configurable ExchangeManager #26629
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
9761f56 to
a073be3
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
Personally, I don't think that we want change like that. It doesn't solve any particular issue, at least none that is highlighted in that PR. How does it make deployment more flexible? |
wendigo
left a comment
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.
.
a073be3 to
aa59d0d
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
aa59d0d to
985c156
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
I agree. |
Hi @wendigo @losipiuk Thanks for reviewing the PR! Let me clarify the motivation — the goal is to make deployments in containerized/Kubernetes environments more flexible. issue link: #26611 Today ExchangeManagerRegistry always looks for In Kubernetes, configuration is usually mounted from Secrets or Vault Agent sidecars, and the target path is not always /etc/trino (sometimes it’s a read-only volume or gets replaced by the image build). Helm charts and operators often template configs into arbitrary paths; to use exchange manager we currently have to copy/patch them into /etc/trino, which breaks immutability and complicates upgrades. Other Trino subsystems (e.g. access-control.config-files, event-listener.config-files) already allow a custom location; but this component still relies on a hardcoded path. By adding an optional property So the change doesn’t affect default users, but gives operators more flexibility and aligns this component with the rest of the configuration system. Would that address the concern about “no particular issue”? Happy to update the PR description. |
|
Hi @ebyhr Documentation has been added to reflect the new exchange-manager.config-file option, please help review, thank you! |
985c156 to
9267001
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
core/trino-main/src/main/java/io/trino/exchange/ExchangeManagerRegistry.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/exchange/ExchangeManagerConfig.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/exchange/ExchangeManagerConfig.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/exchange/TestExchangeConfig.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/exchange/TestExchangeConfig.java
Outdated
Show resolved
Hide resolved
9267001 to
4c6f571
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
core/trino-main/src/main/java/io/trino/exchange/ExchangeManagerConfig.java
Outdated
Show resolved
Hide resolved
4c6f571 to
b66e1db
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
|
@cla-bot check |
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
The cla-bot has been summoned, and re-checked this pull request! |
Add support for specifying exchange manager config file location in config.
b66e1db to
232ca62
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
rebsed |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
Description
This PR adds support for configuring the location of the Exchange Manager configuration file.
Users can now specify a custom path for the Exchange Manager config file, which allows for more flexible deployment and easier configuration management.
Additional context and related issues
Closes #26611
#26611
Release notes
(x) Release notes are required, with the following suggested text: