Skip to content

Load fundamental modules by default #597

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.

Sign up for GitHub

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

Merged
oneonestar merged 5 commits into from
Jan 24, 2025
Merged

Load fundamental modules by default #597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ed94dbf
Load ClusterStatsMonitorModule by default
oneonestar Jan 21, 2025
6623c03
Load ClusterStateListenerModule by default
oneonestar Jan 21, 2025
760485d
Load HaGatewayProviderModule by default
oneonestar Jan 21, 2025
468e2b6
Load ActiveClusterMonitor by default
oneonestar Jan 21, 2025
d502895
Add checks to prevent loading default modules in config
oneonestar Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions docs/operation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,6 @@ to config file's modules section like below

```yaml
modules:
- io.trino.gateway.ha.module.HaGatewayProviderModule
- io.trino.gateway.ha.module.ClusterStateListenerModule
- io.trino.gateway.ha.module.ClusterStatsMonitorModule
- io.trino.gateway.ha.module.QueryCountBasedRouterProvider
```
- The router works on the stats it receives from the clusters about the load i.e number queries queued and running on a cluster at regular intervals which can be configured like below. The default interval is 1 min
Expand Down
8 changes: 0 additions & 8 deletions docs/quickstart-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,3 @@ dataStore:

clusterStatsConfiguration:
monitorType: INFO_API

modules:
- io.trino.gateway.ha.module.HaGatewayProviderModule
- io.trino.gateway.ha.module.ClusterStateListenerModule
- io.trino.gateway.ha.module.ClusterStatsMonitorModule

managedApps:
- io.trino.gateway.ha.clustermonitor.ActiveClusterMonitor
8 changes: 0 additions & 8 deletions gateway-ha/gateway-ha-config-docker.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,3 @@ dataStore:

clusterStatsConfiguration:
monitorType: INFO_API

modules:
- io.trino.gateway.ha.module.HaGatewayProviderModule
- io.trino.gateway.ha.module.ClusterStateListenerModule
- io.trino.gateway.ha.module.ClusterStatsMonitorModule

managedApps:
- io.trino.gateway.ha.clustermonitor.ActiveClusterMonitor
8 changes: 0 additions & 8 deletions gateway-ha/gateway-ha-config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,3 @@ clusterStatsConfiguration:
# This can be adjusted based on the coordinator state
monitor:
taskDelaySeconds: 10

modules:
- io.trino.gateway.ha.module.HaGatewayProviderModule
- io.trino.gateway.ha.module.ClusterStateListenerModule
- io.trino.gateway.ha.module.ClusterStatsMonitorModule

managedApps:
- io.trino.gateway.ha.clustermonitor.ActiveClusterMonitor
38 changes: 30 additions & 8 deletions gateway-ha/src/main/java/io/trino/gateway/baseapp/BaseApp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@
*/
package io.trino.gateway.baseapp;

import com.google.common.collect.ImmutableList;
import com.google.inject.Binder;
import com.google.inject.Module;
import com.google.inject.Scopes;
import io.airlift.log.Logger;
import io.trino.gateway.ha.clustermonitor.ActiveClusterMonitor;
import io.trino.gateway.ha.clustermonitor.ForMonitor;
import io.trino.gateway.ha.config.HaGatewayConfiguration;
import io.trino.gateway.ha.handler.ProxyHandlerStats;
Expand Down Expand Up @@ -86,7 +88,7 @@ private static Module newModule(String clazz, HaGatewayConfiguration configurati
return null;
}

private static void validateModules(List<Module> modules, HaGatewayConfiguration configuration)
private static void addDefaultRouterProviderModules(List<Module> modules, HaGatewayConfiguration configuration)
{
Optional<Module> routerProvider = modules.stream()
.filter(module -> module instanceof RouterBaseModule)
Expand All @@ -102,22 +104,37 @@ public static List<Module> addModules(HaGatewayConfiguration configuration)
{
List<Module> modules = new ArrayList<>();
if (configuration.getModules() == null) {
logger.warn("No modules to load.");
return modules;
logger.info("No modules to load.");
}
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

warnLoadingDefaultModules(clazz);
modules.add(newModule(clazz, configuration));
}
}

validateModules(modules, configuration);
addDefaultRouterProviderModules(modules, configuration);

return modules;
}

private static void warnLoadingDefaultModules(String clazz)
{
// Remove this check when user finished the migration
List<String> defaultModules = ImmutableList.of(
"io.trino.gateway.ha.module.HaGatewayProviderModule",
"io.trino.gateway.ha.module.ClusterStateListenerModule",
"io.trino.gateway.ha.module.ClusterStatsMonitorModule");
if (defaultModules.contains(clazz)) {
logger.error("Default module [%s] is already being loaded. Please remove it from the config file", clazz);
System.exit(1);
}
}

@Override
public void configure(Binder binder)
{
binder.bind(HaGatewayConfiguration.class).toInstance(configuration);
binder.bind(ActiveClusterMonitor.class).in(Scopes.SINGLETON);
registerAuthFilters(binder);
registerResources(binder);
registerProxyResources(binder);
Expand All @@ -131,11 +148,16 @@ public void configure(Binder binder)
private static void addManagedApps(HaGatewayConfiguration configuration, Binder binder)
{
if (configuration.getManagedApps() == null) {
logger.error("No managed apps found");
logger.info("No managed apps found");
return;
}
configuration.getManagedApps().forEach(
clazz -> {
// Remove this check when user finished the migration
if (clazz.equals("io.trino.gateway.ha.clustermonitor.ActiveClusterMonitor")) {
logger.error("Default class ActiveClusterMonitor is already being loaded. Please remove it from the config file");
System.exit(1);
}
try {
Class<?> c = Class.forName(clazz);
binder.bind(c).in(Scopes.SINGLETON);
Expand Down
2 changes: 2 additions & 0 deletions gateway-ha/src/main/java/io/trino/gateway/ha/HaGatewayLauncher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import io.airlift.units.Duration;
import io.trino.gateway.baseapp.BaseApp;
import io.trino.gateway.ha.config.HaGatewayConfiguration;
import io.trino.gateway.ha.module.HaGatewayProviderModule;
import io.trino.gateway.ha.persistence.FlywayMigration;
import org.weakref.jmx.guice.MBeanModule;

Expand Down Expand Up @@ -65,6 +66,7 @@ private void start(List<Module> additionalModules, HaGatewayConfiguration config
new JsonModule(),
new JaxrsModule(),
new TracingModule("trino-gateway", version),
new HaGatewayProviderModule(configuration),
new BaseApp(configuration));
modules.addAll(additionalModules);

Expand Down
62 changes: 0 additions & 62 deletions gateway-ha/src/main/java/io/trino/gateway/ha/module/ClusterStateListenerModule.java
View file
Open in desktop

This file was deleted.

56 changes: 0 additions & 56 deletions gateway-ha/src/main/java/io/trino/gateway/ha/module/ClusterStatsMonitorModule.java
View file
Open in desktop

This file was deleted.

49 changes: 49 additions & 0 deletions gateway-ha/src/main/java/io/trino/gateway/ha/module/HaGatewayProviderModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,33 @@

import com.google.common.collect.ImmutableList;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
import com.google.inject.Provides;
import com.google.inject.Singleton;
import io.airlift.http.client.HttpClient;
import io.trino.gateway.ha.clustermonitor.ClusterStatsHttpMonitor;
import io.trino.gateway.ha.clustermonitor.ClusterStatsInfoApiMonitor;
import io.trino.gateway.ha.clustermonitor.ClusterStatsJdbcMonitor;
import io.trino.gateway.ha.clustermonitor.ClusterStatsMonitor;
import io.trino.gateway.ha.clustermonitor.ClusterStatsObserver;
import io.trino.gateway.ha.clustermonitor.ForMonitor;
import io.trino.gateway.ha.clustermonitor.HealthCheckObserver;
import io.trino.gateway.ha.clustermonitor.NoopClusterStatsMonitor;
import io.trino.gateway.ha.clustermonitor.TrinoClusterStatsObserver;
import io.trino.gateway.ha.config.AuthenticationConfiguration;
import io.trino.gateway.ha.config.AuthorizationConfiguration;
import io.trino.gateway.ha.config.ClusterStatsConfiguration;
import io.trino.gateway.ha.config.GatewayCookieConfigurationPropertiesProvider;
import io.trino.gateway.ha.config.HaGatewayConfiguration;
import io.trino.gateway.ha.config.MonitorConfiguration;
import io.trino.gateway.ha.config.OAuth2GatewayCookieConfigurationPropertiesProvider;
import io.trino.gateway.ha.config.RoutingRulesConfiguration;
import io.trino.gateway.ha.config.RulesExternalConfiguration;
import io.trino.gateway.ha.config.UserConfiguration;
import io.trino.gateway.ha.router.BackendStateManager;
import io.trino.gateway.ha.router.ForRouter;
import io.trino.gateway.ha.router.RoutingGroupSelector;
import io.trino.gateway.ha.router.RoutingManager;
import io.trino.gateway.ha.security.ApiAuthenticator;
import io.trino.gateway.ha.security.AuthorizationManager;
import io.trino.gateway.ha.security.BasicAuthFilter;
Expand All @@ -46,6 +59,7 @@
import io.trino.gateway.ha.security.util.ChainedAuthFilter;
import jakarta.ws.rs.container.ContainerRequestFilter;

import java.util.List;
import java.util.Map;

import static io.airlift.jaxrs.JaxrsBinder.jaxrsBinder;
Expand All @@ -67,6 +81,7 @@ protected void configure()
jaxrsBinder(binder()).bindInstance(resourceSecurityDynamicFeature);
}

@Inject
public HaGatewayProviderModule(HaGatewayConfiguration configuration)
{
this.configuration = requireNonNull(configuration, "configuration is null");
Expand Down Expand Up @@ -200,4 +215,38 @@ public RoutingGroupSelector getRoutingGroupSelector(@ForRouter HttpClient httpCl
}
return RoutingGroupSelector.byRoutingGroupHeader();
}

@Provides
@Singleton
public ClusterStatsMonitor getClusterStatsMonitor(@ForMonitor HttpClient httpClient)
{
ClusterStatsConfiguration clusterStatsConfig = configuration.getClusterStatsConfiguration();
if (configuration.getBackendState() == null) {
return new ClusterStatsInfoApiMonitor(httpClient, configuration.getMonitor());
}
return switch (clusterStatsConfig.getMonitorType()) {
case INFO_API -> new ClusterStatsInfoApiMonitor(httpClient, configuration.getMonitor());
case UI_API -> new ClusterStatsHttpMonitor(configuration.getBackendState());
case JDBC -> new ClusterStatsJdbcMonitor(configuration.getBackendState(), configuration.getMonitor());
case NOOP -> new NoopClusterStatsMonitor();
};
}

@Provides
@Singleton
public List<TrinoClusterStatsObserver> getClusterStatsObservers(
RoutingManager mgr,
BackendStateManager backendStateManager)
{
return ImmutableList.<TrinoClusterStatsObserver>builder()
.add(new HealthCheckObserver(mgr))
.add(new ClusterStatsObserver(backendStateManager))
.build();
}

@Provides
public MonitorConfiguration getMonitorConfiguration()
{
return configuration.getMonitor();
}
}
3 changes: 0 additions & 3 deletions gateway-ha/src/test/resources/auth/auth-test-config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,6 @@ dataStore:
driver: org.h2.Driver
runMigrationsEnabled: false

modules:
- io.trino.gateway.ha.module.HaGatewayProviderModule

extraWhitelistPaths:
- '/v1/custom.*'

Expand Down
3 changes: 0 additions & 3 deletions gateway-ha/src/test/resources/auth/oauth-test-config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@ dataStore:
driver: org.h2.Driver
runMigrationsEnabled: false

modules:
- io.trino.gateway.ha.module.HaGatewayProviderModule

extraWhitelistPaths:
- '/v1/custom.*'

Expand Down
8 changes: 0 additions & 8 deletions gateway-ha/src/test/resources/test-config-template.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,6 @@ dataStore:
driver: org.h2.Driver
runMigrationsEnabled: false

modules:
- io.trino.gateway.ha.module.HaGatewayProviderModule
- io.trino.gateway.ha.module.ClusterStateListenerModule
- io.trino.gateway.ha.module.ClusterStatsMonitorModule

managedApps:
- io.trino.gateway.ha.clustermonitor.ActiveClusterMonitor

clusterStatsConfiguration:
monitorType: INFO_API

Expand Down
Loading