Copilot commented on code in PR #19248:
URL: https://github.com/apache/druid/pull/19248#discussion_r3031468425
##########
pom.xml:
##########
@@ -1014,6 +1029,16 @@
<artifactId>httpclient</artifactId>
<version>${httpclient.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.httpcomponents.client5</groupId>
+ <artifactId>httpclient5</artifactId>
+ <version>5.5.1</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.httpcomponents.core5</groupId>
+ <artifactId>httpcore5</artifactId>
+ <version>5.3.6</version>
+ </dependency>
Review Comment:
`httpclient5` / `httpcore5` are added to dependencyManagement, but
`licenses.yaml` only registers Apache HttpClient/HttpCore 4.x today and does
not include the client5/core5 artifacts (or their versions). If these v5
artifacts are present in dependency reports, the license check will report them
as missing. Add matching `licenses.yaml` entries for
`org.apache.httpcomponents.client5:httpclient5` (5.5.1) and
`org.apache.httpcomponents.core5:httpcore5` (5.3.6).
##########
sql/src/test/java/org/apache/druid/sql/calcite/SqlTestFrameworkConfig.java:
##########
@@ -252,13 +253,24 @@ public SqlTestFrameworkConfigStore(
this.queryComponentSupplierWrapper = queryComponentSupplierWrapper;
}
- Map<SqlTestFrameworkConfig, ConfigurationInstance> configMap = new
HashMap<>();
+ Map<SqlTestFrameworkConfig, ConfigurationInstance> configMap = new
LinkedHashMap<>(16, 0.75f, true)
+ {
+ @Override
+ protected boolean removeEldestEntry(Map.Entry<SqlTestFrameworkConfig,
ConfigurationInstance> eldest)
+ {
+ if (size() > MAX_CACHED_CONFIGS) {
+ eldest.getValue().close();
+ return true;
+ }
+ return false;
+ }
Review Comment:
The LRU eviction closes the eldest ConfigurationInstance as soon as the
cache exceeds 32 entries. Since ConfigurationInstance.framework may still be in
use by a running test (or another thread if tests execute concurrently while
sharing the same Rule/store), this can lead to flaky failures due to resources
being closed mid-test. Consider avoiding closing on eviction (only close on
SqlTestFrameworkConfigStore.close()), or introduce a safe lifecycle mechanism
(e.g., per-test/per-thread store, reference counting, or eviction only of
instances known to be unused).
##########
pom.xml:
##########
@@ -484,6 +484,21 @@
<artifactId>logging-interceptor</artifactId>
<version>${okhttp.version}</version>
</dependency>
+ <dependency>
+ <groupId>com.squareup.okio</groupId>
+ <artifactId>okio</artifactId>
+ <version>3.16.4</version>
+ </dependency>
+ <dependency>
+ <groupId>redis.clients</groupId>
+ <artifactId>jedis</artifactId>
+ <version>7.0.0</version>
+ </dependency>
+ <dependency>
+ <groupId>com.github.fppt</groupId>
+ <artifactId>jedis-mock</artifactId>
+ <version>1.1.12</version>
+ </dependency>
Review Comment:
This change introduces dependencyManagement entries for
`redis.clients:jedis` (7.0.0) and `com.github.fppt:jedis-mock` (1.1.12), but
`licenses.yaml` does not currently register these coordinates. If these
dependencies appear in the generated dependency reports,
`distribution/bin/check-licenses.py` will fail with missing license registry
entries. Please add corresponding `licenses.yaml` entries (or mark them as
skipped if appropriate for the build outputs).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
