Skip to content

Load fundamental modules by default#597

Merged
oneonestar merged 5 commits intotrinodb:mainfrom
oneonestar:star/cleanup_module
Jan 24, 2025
Merged

Load fundamental modules by default#597
oneonestar merged 5 commits intotrinodb:mainfrom
oneonestar:star/cleanup_module

Conversation

@oneonestar
Copy link
Copy Markdown
Member

@oneonestar oneonestar commented Jan 22, 2025

Description

Load some fundamental modules by default.
HaGatewayProviderModule provides core functions for gateway including auth and routing.
ActiveClusterMonitor perform health check on Trino cluster.
ClusterStateListenerModule and ClusterStatsMonitorModule are required for ActiveClusterMonitor.

Release notes

(x) Release notes are required, with the following suggested text:

*  [:warning: Breaking change:](#breaking) Load fundamental modules by default. 
Remove `HaGatewayProviderModule`, `ActiveClusterMonitor`, `ClusterStateListenerModule`,
and `ClusterStatsMonitorModule` from config file. ({issue}`issuenumber`)

@cla-bot cla-bot Bot added the cla-signed label Jan 22, 2025
@oneonestar oneonestar changed the title Load modules by default Load fundamental modules by default Jan 22, 2025
willmostly
willmostly requested changes Jan 22, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

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

Looks good, just two small comments

Comment thread gateway-ha/src/main/java/io/trino/gateway/baseapp/BaseApp.java Outdated
Comment thread gateway-ha/src/main/java/io/trino/gateway/baseapp/BaseApp.java
for (String clazz : configuration.getModules()) {
modules.add(newModule(clazz, configuration));
else {
for (String clazz : configuration.getModules()) {
Copy link
Copy Markdown
Contributor

@willmostly willmostly Jan 22, 2025

Choose a reason for hiding this comment

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

Will there be an error if the configured modules includes one of the default modules? If so we should make sure there is a clear message telling the user how to fix the situation.

Copy link
Copy Markdown
Member Author

@oneonestar oneonestar Jan 23, 2025

Choose a reason for hiding this comment

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

I added checks to prevent loading default modules in the config. Instead of trying to provide compatibility and issuing a warning, it will fail to start if any of the default modules are set in the config file. It might be a bit aggressive. I'm okay with using the former method if you prefer.

Copy link
Copy Markdown
Contributor

@willmostly willmostly Jan 23, 2025

Choose a reason for hiding this comment

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

I think this approach is correct. It is better to force a one-time fix than to try to continue supporting a broken configuration indefinitely

willmostly
willmostly approved these changes Jan 23, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment thread gateway-ha/src/main/java/io/trino/gateway/baseapp/BaseApp.java
for (String clazz : configuration.getModules()) {
modules.add(newModule(clazz, configuration));
else {
for (String clazz : configuration.getModules()) {
Copy link
Copy Markdown
Contributor

@willmostly willmostly Jan 23, 2025

Choose a reason for hiding this comment

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

I think this approach is correct. It is better to force a one-time fix than to try to continue supporting a broken configuration indefinitely

@oneonestar oneonestar merged commit 26dc2fe into trinodb:main Jan 24, 2025
@oneonestar oneonestar deleted the star/cleanup_module branch January 24, 2025 04:32
@github-actions github-actions Bot added this to the 14 milestone Jan 24, 2025
@oneonestar oneonestar mentioned this pull request Jan 27, 2025
Merged
@mosabua mosabua mentioned this pull request Jan 28, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@willmostly willmostly willmostly approved these changes

Assignees

No one assigned

Labels

cla-signed

Milestone

14

Development

Successfully merging this pull request may close these issues.

2 participants

@oneonestar @willmostly