Fix ErrorProne warnings and OAuth test timing issues #790
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Replace deprecated URL(String) constructor with URI.create().toURL() - Fix FutureReturnValueIgnored warnings by capturing return values with @SuppressWarnings("unused") - Replace String.split() with Guava Splitter to avoid StringSplitter warnings - Use pattern-matching instanceof to simplify type checks - Fix time access in static initializer by moving to instance field - Fix integer overflow by adding explicit long suffix (1000L) - Use Long.compareTo() instead of manual subtraction to avoid sign flip - Replace Math.abs() with ThreadLocalRandom.nextInt() for safer random number generation - Suppress TestContainers deprecation warnings with @SuppressWarnings("deprecation") - Replace Boolean.TRUE/FALSE with boolean literals in test assertions - Add multi-byte read method override to fix InputStreamSlowMultibyteRead warning - Replace Iterables.get() with Stream API for modernizer compliance - Add 10-second clock skew tolerance to JWT verification for containerized environments
Reviewer's GuideThis PR addresses ErrorProne warnings and modernizes the code by replacing deprecated APIs, adopting Java pattern matching, fixing timing and overflow issues in production and test code, and enhancing OAuth test stability with SSL trust and clock skew tolerance. Sequence diagram for scheduled task handling with Future return value capturesequenceDiagram
participant ActiveClusterMonitor
participant ScheduledExecutor
ActiveClusterMonitor->>ScheduledExecutor: scheduleAtFixedRate(task)
ScheduledExecutor-->>ActiveClusterMonitor: returns Future
ActiveClusterMonitor->>ActiveClusterMonitor: captures Future in unused variable (suppressed warning)
participant ClusterMetricsStatsExporter
participant ScheduledExecutor
ClusterMetricsStatsExporter->>ScheduledExecutor: scheduleAtFixedRate(task)
ScheduledExecutor-->>ClusterMetricsStatsExporter: returns Future
ClusterMetricsStatsExporter->>ClusterMetricsStatsExporter: captures Future in unused variable (suppressed warning)
participant JdbcConnectionManager
participant ExecutorService
JdbcConnectionManager->>ExecutorService: scheduleWithFixedDelay(cleanupTask)
ExecutorService-->>JdbcConnectionManager: returns Future
JdbcConnectionManager->>JdbcConnectionManager: captures Future in unused variable (suppressed warning)
Class diagram for updated TrinoRequestUser and related utility usageclassDiagram
class TrinoRequestUser {
+Optional<String> user
+Optional<UserInfo> userInfo
+Optional<LoadingCache<String, UserInfo>> userInfoCache
-extractUserFromAuthorizationHeader(header: String, userField: String): Optional<String>
}
class UserInfo
TrinoRequestUser --> UserInfo
TrinoRequestUser --> LoadingCache
class ProxyUtils {
+extractQueryIdIfPresent(path: String, query: String): Optional<String>
}
class TrinoQueryProperties {
-getPreparedStatements(headers: Enumeration<String>): Map<String, String>
}
class StatementUtils {
+getResourceGroupQueryType(statement: Statement): String
}
StatementUtils --> Statement
Statement <|-- ExplainAnalyze
class LbKeyProvider {
+getRsaPrivateKey(): RSAPrivateKey
+getRsaPublicKey(): RSAPublicKey
}
LbKeyProvider --> RSAPrivateKey
LbKeyProvider --> RSAPublicKey
class GatewayCookie {
+compareTo(o: GatewayCookie): int
}
GatewayCookie --> GatewayCookie
class StochasticRoutingManager {
+selectBackend(backends: List<ProxyBackendConfiguration>): Optional<ProxyBackendConfiguration>
}
StochasticRoutingManager --> ProxyBackendConfiguration
class LbTokenUtil {
+validateToken(idToken: String, publicKey: RSAPublicKey, audience: String): boolean
}
LbTokenUtil --> RSAPublicKey
Class diagram for GatewayWebAppResource timing field updateclassDiagram
class GatewayWebAppResource {
-startTime: LocalDateTime (instance field)
-formatter: DateTimeFormatter
-getDistribution(query: QueryDistributionRequest): Response
}
GatewayWebAppResource --> QueryDistributionRequest
GatewayWebAppResource --> Response
Class diagram for StochasticRoutingManager random backend selection updateclassDiagram
class StochasticRoutingManager {
+selectBackend(backends: List<ProxyBackendConfiguration>): Optional<ProxyBackendConfiguration>
}
StochasticRoutingManager --> ProxyBackendConfiguration
Class diagram for LbTokenUtil JWT verification with clock skew toleranceclassDiagram
class LbTokenUtil {
+validateToken(idToken: String, publicKey: RSAPublicKey, audience: String): boolean
}
LbTokenUtil --> RSAPublicKey
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ebyhr
requested changes
Nov 4, 2025
Member
ebyhr
left a comment
ebyhr
left a comment
There was a problem hiding this comment.
Please split into some commits since the commit contains too many changes.
| log.info("Running cluster monitor with connection task delay of %s", taskDelay); | ||
| scheduledExecutor.scheduleAtFixedRate(() -> { | ||
| @SuppressWarnings("unused") | ||
| var unused = scheduledExecutor.scheduleAtFixedRate(() -> { |
Member
There was a problem hiding this comment.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
And anyway, I think var _ should be an exception to the "no var" rule.
|
|
||
| private final Optional<LoadingCache<String, UserInfo>> userInfoCache; | ||
| private Optional<String> user = Optional.empty(); | ||
| private Optional<UserInfo> userInfo = Optional.empty(); |
Member
There was a problem hiding this comment.
What was the errorprone warning on this change?
| public class GatewayWebAppResource | ||
| { | ||
| private static final LocalDateTime START_TIME = LocalDateTime.now(ZoneId.systemDefault()); | ||
| private final LocalDateTime startTime = LocalDateTime.now(ZoneId.systemDefault()); |
Member
There was a problem hiding this comment.
What was the errorprone warning on this constant?
| 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); |
Member
There was a problem hiding this comment.
Use isTrue() instead. Same for others.
| 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); |
Member
There was a problem hiding this comment.
Use isFalse() instead. Same for others.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fix error messages during mvn build and test timing issues
Additional context and related issues
Currently, mvn build logs warning
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required, with the following suggested text:
* Fix some things.Summary by Sourcery
Address ErrorProne warnings and OAuth test timing issues by refactoring deprecated APIs, adopting safer patterns, suppressing spurious warnings, and adjusting JWT verification tolerance
Bug Fixes:
Enhancements:
Tests: