Skip to content

Fix ErrorProne warnings and OAuth test timing issues #790

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix ErrorProne warnings and OAuth test timing issues #790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 1 deletion gateway-ha/src/main/java/io/trino/gateway/ha/clustermonitor/ActiveClusterMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ public ActiveClusterMonitor(
public void start()
{
log.info("Running cluster monitor with connection task delay of %s", taskDelay);
scheduledExecutor.scheduleAtFixedRate(() -> {
@SuppressWarnings("unused")
var unused = scheduledExecutor.scheduleAtFixedRate(() -> {
Copy link
Member

@ebyhr ebyhr Nov 4, 2025

Choose a reason for hiding this comment

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

https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#avoid-var

Copy link

@ksobolew ksobolew Nov 4, 2025

Choose a reason for hiding this comment

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

Also, we can do Future<?> _ = ... instead of suppressing the unused warning. This is a modern Java way of saying "I don't care about this value".

Copy link

@ksobolew ksobolew Nov 4, 2025

Choose a reason for hiding this comment

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

And anyway, I think var _ should be an exception to the "no var" rule.

try {
log.info("Getting stats for all active clusters");
List<ProxyBackendConfiguration> activeClusters =
Expand Down
3 changes: 2 additions & 1 deletion gateway-ha/src/main/java/io/trino/gateway/ha/clustermonitor/ClusterMetricsStatsExporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ public ClusterMetricsStatsExporter(GatewayBackendManager gatewayBackendManager,
public void start()
{
log.debug("Running periodic metric refresh with interval of %s", refreshInterval);
scheduledExecutor.scheduleAtFixedRate(() -> {
@SuppressWarnings("unused")
var unused = scheduledExecutor.scheduleAtFixedRate(() -> {
try {
updateClustersMetricRegistry();
}
Expand Down
3 changes: 2 additions & 1 deletion gateway-ha/src/main/java/io/trino/gateway/ha/clustermonitor/ClusterStatsJdbcMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import io.trino.gateway.ha.config.ProxyBackendConfiguration;

import java.net.MalformedURLException;
import java.net.URI;
import java.net.URL;
import java.sql.Connection;
import java.sql.DriverManager;
Expand Down Expand Up @@ -69,7 +70,7 @@ public ClusterStats monitor(ProxyBackendConfiguration backend)
ClusterStats.Builder clusterStats = ClusterStatsMonitor.getClusterStatsBuilder(backend);
String jdbcUrl;
try {
URL parsedUrl = new URL(url);
URL parsedUrl = URI.create(url).toURL();
jdbcUrl = String
.format("jdbc:trino://%s:%s/system",
parsedUrl.getHost(),
Expand Down
3 changes: 2 additions & 1 deletion gateway-ha/src/main/java/io/trino/gateway/ha/clustermonitor/ClusterStatsMetricsMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package io.trino.gateway.ha.clustermonitor;

import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import io.airlift.http.client.HttpClient;
Expand Down Expand Up @@ -190,7 +191,7 @@ public Map<String, String> handle(Request request, Response response)
String responseBody = new String(response.getInputStream().readAllBytes(), UTF_8);
Map<String, String> metrics = Arrays.stream(responseBody.split("\n"))
.filter(line -> !line.startsWith("#"))
.collect(toImmutableMap(s -> s.split(" ")[0], s -> s.split(" ")[1]));
.collect(toImmutableMap(s -> Splitter.on(' ').splitToList(s).get(0), s -> Splitter.on(' ').splitToList(s).get(1)));
if (!metrics.keySet().containsAll(requiredKeys)) {
throw new UnexpectedResponseException(
format("Request is missing required keys: \n%s\nin response: '%s'", String.join("\n", requiredKeys), responseBody),
Expand Down
15 changes: 8 additions & 7 deletions gateway-ha/src/main/java/io/trino/gateway/ha/handler/ProxyUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package io.trino.gateway.ha.handler;

import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableSet;
import com.google.common.io.CharStreams;
import io.airlift.log.Logger;
Expand Down Expand Up @@ -103,15 +104,15 @@ public static Optional<String> extractQueryIdIfPresent(String path, String query
}
if (matchingStatementPath.isPresent() || path.startsWith(V1_QUERY_PATH)) {
path = path.replace(matchingStatementPath.orElse(V1_QUERY_PATH), "");
String[] tokens = path.split("/");
if (tokens.length >= 2) {
if (tokens.length >= 3 && QUERY_STATE_PATH.contains(tokens[1])) {
if (tokens.length >= 4 && tokens[2].equals(PARTIAL_CANCEL_PATH)) {
return Optional.of(tokens[3]);
List<String> tokens = Splitter.on('/').splitToList(path);
if (tokens.size() >= 2) {
if (tokens.size() >= 3 && QUERY_STATE_PATH.contains(tokens.get(1))) {
if (tokens.size() >= 4 && tokens.get(2).equals(PARTIAL_CANCEL_PATH)) {
return Optional.of(tokens.get(3));
}
return Optional.of(tokens[2]);
return Optional.of(tokens.get(2));
}
return Optional.of(tokens[1]);
return Optional.of(tokens.get(1));
}
}
else if (path.startsWith(TRINO_UI_PATH)) {
Expand Down
3 changes: 2 additions & 1 deletion gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcConnectionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ private static URI getUriWithRoutingGroupDatabase(String routingGroupDatabase, i

private void startCleanUps()
{
executorService.scheduleWithFixedDelay(
@SuppressWarnings("unused")
var unused = executorService.scheduleWithFixedDelay(
() -> {
log.info("Performing query history cleanup task");
long created = System.currentTimeMillis() - TimeUnit.HOURS.toMillis(this.configuration.getQueryHistoryHoursRetention());
Expand Down
6 changes: 3 additions & 3 deletions gateway-ha/src/main/java/io/trino/gateway/ha/resource/GatewayWebAppResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@
@Path("/webapp")
public class GatewayWebAppResource
{
private static final LocalDateTime START_TIME = LocalDateTime.now(ZoneId.systemDefault());
private final LocalDateTime startTime = LocalDateTime.now(ZoneId.systemDefault());
Copy link
Member

@ebyhr ebyhr Nov 4, 2025

Choose a reason for hiding this comment

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

What was the errorprone warning on this constant?

private static final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSXXX");
private final GatewayBackendManager gatewayBackendManager;
private final QueryHistoryManager queryHistoryManager;
Expand Down Expand Up @@ -167,7 +167,7 @@ public Response getDistribution(QueryDistributionRequest query)
state -> state.trinoStatus() == TrinoStatus.HEALTHY,
Collectors.collectingAndThen(Collectors.counting(), Long::intValue)));
Integer latestHour = query.latestHour();
Long ts = System.currentTimeMillis() - (latestHour * 60 * 60 * 1000);
Long ts = System.currentTimeMillis() - (latestHour * 60 * 60 * 1000L);
List<DistributionResponse.LineChart> lineChart = queryHistoryManager.findDistribution(ts);
lineChart.forEach(qh -> qh.setName(urlToNameMap.get(qh.getBackendUrl())));
Map<String, List<DistributionResponse.LineChart>> lineChartMap = lineChart.stream().collect(Collectors.groupingBy(DistributionResponse.LineChart::getName));
Expand All @@ -192,7 +192,7 @@ public Response getDistribution(QueryDistributionRequest query)
distributionResponse.setTotalQueryCount(totalQueryCount);
distributionResponse.setAverageQueryCountSecond(totalQueryCount / (latestHour * 60d * 60d));
distributionResponse.setAverageQueryCountMinute(totalQueryCount / (latestHour * 60d));
ZonedDateTime zonedLocalTime = START_TIME.atZone(ZoneId.systemDefault());
ZonedDateTime zonedLocalTime = startTime.atZone(ZoneId.systemDefault());
ZonedDateTime utcTime = zonedLocalTime.withZoneSameInstant(ZoneOffset.UTC);
distributionResponse.setStartTime(utcTime.format(formatter));
return Response.ok(Result.ok(distributionResponse)).build();
Expand Down
2 changes: 1 addition & 1 deletion gateway-ha/src/main/java/io/trino/gateway/ha/router/GatewayCookie.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ private String computeSignature()
public int compareTo(GatewayCookie o)
{
int priorityDelta = unsignedGatewayCookie.getPriority() - o.getPriority();
return priorityDelta != 0 ? priorityDelta : (int) (unsignedGatewayCookie.getTs() - o.getTs());
return priorityDelta != 0 ? priorityDelta : unsignedGatewayCookie.getTs().compareTo(o.getTs());
}

public Cookie toCookie()
Expand Down
4 changes: 2 additions & 2 deletions gateway-ha/src/main/java/io/trino/gateway/ha/router/StatementUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,8 @@ private StatementUtils() {}

public static String getResourceGroupQueryType(Statement statement)
{
if (statement instanceof ExplainAnalyze) {
return getResourceGroupQueryType(((ExplainAnalyze) statement).getStatement());
if (statement instanceof ExplainAnalyze explainAnalyze) {
return getResourceGroupQueryType(explainAnalyze.getStatement());
}
StatementTypeInfo<? extends Statement> statementTypeInfo = STATEMENT_QUERY_TYPES.get(statement.getClass());
if (statementTypeInfo != null) {
Expand Down
6 changes: 2 additions & 4 deletions gateway-ha/src/main/java/io/trino/gateway/ha/router/StochasticRoutingManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,11 @@

import java.util.List;
import java.util.Optional;
import java.util.Random;
import java.util.concurrent.ThreadLocalRandom;

public class StochasticRoutingManager
extends BaseRoutingManager
{
private static final Random RANDOM = new Random();

@Inject
public StochasticRoutingManager(
GatewayBackendManager gatewayBackendManager,
Expand All @@ -41,7 +39,7 @@ protected Optional<ProxyBackendConfiguration> selectBackend(List<ProxyBackendCon
if (backends.isEmpty()) {
return Optional.empty();
}
int backendId = Math.abs(RANDOM.nextInt()) % backends.size();
int backendId = ThreadLocalRandom.current().nextInt(backends.size());
return Optional.of(backends.get(backendId));
}
}
9 changes: 5 additions & 4 deletions gateway-ha/src/main/java/io/trino/gateway/ha/router/TrinoQueryProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.fasterxml.jackson.databind.SerializerProvider;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import com.fasterxml.jackson.databind.ser.std.StdSerializer;
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -300,13 +301,13 @@ private Map<String, String> getPreparedStatements(Enumeration<String> headers)
return preparedStatementsMapBuilder.build();
}
while (headers.hasMoreElements()) {
String[] preparedStatementsArray = headers.nextElement().split(",");
Iterable<String> preparedStatementsArray = Splitter.on(',').split(headers.nextElement());
for (String preparedStatement : preparedStatementsArray) {
String[] nameValue = preparedStatement.split("=");
if (nameValue.length != 2) {
List<String> nameValue = Splitter.on('=').splitToList(preparedStatement);
if (nameValue.size() != 2) {
throw new RequestParsingException(format("preparedStatement must be formatted as name=value, but is %s", preparedStatement));
}
preparedStatementsMapBuilder.put(URLDecoder.decode(nameValue[0], UTF_8), URLDecoder.decode(decodePreparedStatementFromHeader(nameValue[1]), UTF_8));
preparedStatementsMapBuilder.put(URLDecoder.decode(nameValue.get(0), UTF_8), URLDecoder.decode(decodePreparedStatementFromHeader(nameValue.get(1)), UTF_8));
}
}
return preparedStatementsMapBuilder.build();
Expand Down
12 changes: 5 additions & 7 deletions gateway-ha/src/main/java/io/trino/gateway/ha/router/TrinoRequestUser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.fasterxml.jackson.databind.SerializerProvider;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import com.fasterxml.jackson.databind.ser.std.StdSerializer;
import com.google.common.base.Splitter;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
Expand Down Expand Up @@ -54,13 +55,10 @@ public class TrinoRequestUser
public static final String TRINO_USER_HEADER_NAME = "X-Trino-User";
public static final String TRINO_UI_TOKEN_NAME = "Trino-UI-Token";
public static final String TRINO_SECURE_UI_TOKEN_NAME = "__Secure-Trino-ID-Token";

private Optional<String> user = Optional.empty();
private Optional<UserInfo> userInfo = Optional.empty();

private static final Logger log = Logger.get(TrinoRequestUser.class);

private final Optional<LoadingCache<String, UserInfo>> userInfoCache;
private Optional<String> user = Optional.empty();
private Optional<UserInfo> userInfo = Optional.empty();
Copy link
Member

@ebyhr ebyhr Nov 4, 2025

Choose a reason for hiding this comment

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

What was the errorprone warning on this change?


private TrinoRequestUser(ContainerRequestContext request, String userField, Optional<LoadingCache<String, UserInfo>> userInfoCache)
{
Expand Down Expand Up @@ -155,11 +153,11 @@ private Optional<String> extractUserFromAuthorizationHeader(String header, Strin

if (header.contains("Basic")) {
try {
return Optional.of(new String(Base64.getDecoder().decode(header.split(" ")[1]), StandardCharsets.UTF_8).split(":")[0]);
return Optional.of(Splitter.on(':').splitToStream(new String(Base64.getDecoder().decode(Splitter.on(' ').splitToStream(header).skip(1).findFirst().get()), StandardCharsets.UTF_8)).findFirst().get());
}
catch (IllegalArgumentException e) {
log.error(e, "Authorization: Basic header contains invalid base64");
log.debug("Invalid header value: " + header.split(" ")[1]);
log.debug("Invalid header value: " + Splitter.on(' ').splitToStream(header).skip(1).findFirst().get());
return Optional.empty();
}
}
Expand Down
8 changes: 4 additions & 4 deletions gateway-ha/src/main/java/io/trino/gateway/ha/security/LbKeyProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,13 @@ public LbKeyProvider(SelfSignKeyPairConfiguration keypairConfig)

RSAPrivateKey getRsaPrivateKey()
{
return (this.privateKey instanceof RSAPrivateKey)
? (RSAPrivateKey) this.privateKey : null;
return (this.privateKey instanceof RSAPrivateKey rSAPrivateKey)
? rSAPrivateKey : null;
}

RSAPublicKey getRsaPublicKey()
{
return (this.publicKey instanceof RSAPublicKey)
? (RSAPublicKey) this.publicKey : null;
return (this.publicKey instanceof RSAPublicKey rSAPublicKey)
? rSAPublicKey : null;
}
}
3 changes: 2 additions & 1 deletion gateway-ha/src/main/java/io/trino/gateway/ha/security/LbTokenUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ public static boolean validateToken(String idToken, RSAPublicKey publicKey, Stri

audiences.ifPresent(auds -> verification.withAnyOfAudience(auds.toArray(new String[0])));

verification.build().verify(idToken);
// Add clock skew tolerance for containerized environments
verification.acceptLeeway(10).build().verify(idToken);
}
catch (Exception exc) {
log.error(exc, "Could not validate token.");
Expand Down
6 changes: 3 additions & 3 deletions ...-ha/src/test/java/io/trino/gateway/ha/clustermonitor/TestClusterMetricsStatsExporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ void testMetricsRegistrationForNewCluster()
sleepUninterruptibly(2, SECONDS);

verify(statsExporter.exporter()).exportWithGeneratedName(
argThat(stats -> stats instanceof ClusterMetricsStats && ((ClusterMetricsStats) stats).getClusterName().equals(clusterName1)),
argThat(stats -> stats instanceof ClusterMetricsStats clusterMetricsStats && clusterMetricsStats.getClusterName().equals(clusterName1)),
eq(ClusterMetricsStats.class), eq(clusterName1));

// Wait for next update where cluster is added
sleepUninterruptibly(2, SECONDS);

verify(statsExporter.exporter()).exportWithGeneratedName(
argThat(stats -> stats instanceof ClusterMetricsStats && ((ClusterMetricsStats) stats).getClusterName().equals(clusterName2)),
argThat(stats -> stats instanceof ClusterMetricsStats clusterMetricsStats && clusterMetricsStats.getClusterName().equals(clusterName2)),
eq(ClusterMetricsStats.class), eq(clusterName2));
}
}
Expand All @@ -74,7 +74,7 @@ public void testMetricsUnregistrationForRemovedCluster()
sleepUninterruptibly(2, SECONDS);

verify(statsExporter.exporter()).exportWithGeneratedName(
argThat(stats -> stats instanceof ClusterMetricsStats && ((ClusterMetricsStats) stats).getClusterName().equals(clusterName)),
argThat(stats -> stats instanceof ClusterMetricsStats clusterMetricsStats && clusterMetricsStats.getClusterName().equals(clusterName)),
eq(ClusterMetricsStats.class), eq(clusterName));

// Wait for next update where cluster is removed
Expand Down
8 changes: 4 additions & 4 deletions gateway-ha/src/test/java/io/trino/gateway/ha/router/TestResourceGroupsManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ void testReadResourceGroup()
assertThat(resourceGroups.get(0).getName()).isEqualTo("admin");
assertThat(resourceGroups.get(0).getHardConcurrencyLimit()).isEqualTo(20);
assertThat(resourceGroups.get(0).getMaxQueued()).isEqualTo(200);
assertThat(resourceGroups.get(0).getJmxExport()).isEqualTo(Boolean.TRUE);
assertThat(resourceGroups.get(0).getJmxExport()).isEqualTo(true);
Copy link
Member

@ebyhr ebyhr Nov 4, 2025

Choose a reason for hiding this comment

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

Use isTrue() instead. Same for others.

assertThat(resourceGroups.get(0).getSoftMemoryLimit()).isEqualTo("80%");
}

Expand Down Expand Up @@ -128,21 +128,21 @@ void testUpdateResourceGroup()
assertThat(resourceGroups.get(0).getName()).isEqualTo("admin");
assertThat(resourceGroups.get(0).getHardConcurrencyLimit()).isEqualTo(50);
assertThat(resourceGroups.get(0).getMaxQueued()).isEqualTo(50);
assertThat(resourceGroups.get(0).getJmxExport()).isEqualTo(Boolean.FALSE);
assertThat(resourceGroups.get(0).getJmxExport()).isEqualTo(false);
Copy link
Member

@ebyhr ebyhr Nov 4, 2025

Choose a reason for hiding this comment

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

Use isFalse() instead. Same for others.

assertThat(resourceGroups.get(0).getSoftMemoryLimit()).isEqualTo("20%");

assertThat(resourceGroups.get(1).getResourceGroupId()).isEqualTo(2L);
assertThat(resourceGroups.get(1).getName()).isEqualTo("user");
assertThat(resourceGroups.get(1).getHardConcurrencyLimit()).isEqualTo(10);
assertThat(resourceGroups.get(1).getMaxQueued()).isEqualTo(100);
assertThat(resourceGroups.get(1).getJmxExport()).isEqualTo(Boolean.TRUE);
assertThat(resourceGroups.get(1).getJmxExport()).isEqualTo(true);
assertThat(resourceGroups.get(1).getSoftMemoryLimit()).isEqualTo("50%");

assertThat(resourceGroups.get(2).getResourceGroupId()).isEqualTo(3L);
assertThat(resourceGroups.get(2).getName()).isEqualTo("localization-eng");
assertThat(resourceGroups.get(2).getHardConcurrencyLimit()).isEqualTo(50);
assertThat(resourceGroups.get(2).getMaxQueued()).isEqualTo(70);
assertThat(resourceGroups.get(2).getJmxExport()).isEqualTo(Boolean.TRUE);
assertThat(resourceGroups.get(2).getJmxExport()).isEqualTo(true);
assertThat(resourceGroups.get(2).getSoftMemoryLimit()).isEqualTo("20%");
assertThat(resourceGroups.get(2).getSoftConcurrencyLimit()).isEqualTo(Integer.valueOf(20));
}
Expand Down
6 changes: 4 additions & 2 deletions gateway-ha/src/test/java/io/trino/gateway/ha/router/TestRoutingRulesManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ void testConcurrentUpdateRoutingRule()

ExecutorService executorService = Executors.newFixedThreadPool(2);

executorService.submit(() ->
@SuppressWarnings("unused")
var unused1 = executorService.submit(() ->
{
try {
routingRulesManager.updateRoutingRule(routingRule1);
Expand All @@ -138,7 +139,8 @@ void testConcurrentUpdateRoutingRule()
}
});

executorService.submit(() ->
@SuppressWarnings("unused")
var unused2 = executorService.submit(() ->
{
try {
routingRulesManager.updateRoutingRule(routingRule2);
Expand Down
Loading