serhiy-bzhezytskyy commented on code in PR #4612:
URL: https://github.com/apache/solr/pull/4612#discussion_r3534652951
##########
solr/core/build.gradle:
##########
@@ -83,31 +80,25 @@ dependencies {
implementation libs.apache.lucene.sandbox
implementation libs.apache.lucene.spatialextras
implementation libs.apache.lucene.suggest
-
// Collections & lang utilities
implementation libs.google.guava
implementation libs.apache.commons.lang3
implementation libs.apache.commons.math3
implementation libs.commonsio.commonsio
implementation libs.carrotsearch.hppc
-
- implementation(libs.benmanes.caffeine) {transitive = false}
+ api(libs.benmanes.caffeine) {transitive = false}
Review Comment:
You're right that this doesn't belong in this PR — I've reverted it (and the
other external `implementation → api` changes) back to `implementation` here.
This PR is now just the plugin wiring plus safe/mechanical advice; the scope
promotions live in the draft PRs (#4613 for inter-module `project()`, #4614 for
external libs) for exactly the case-by-case discussion you asked for.
One correction to my own earlier framing, in case it's useful: I checked
these against the plugin's ABI dump, and caffeine genuinely *is* exposed in
solr-core's public API today — `CaffeineCache implements
com.github.benmanes.caffeine.cache.RemovalListener`, and `onRemoval(...,
RemovalCause)` is a public method. So it isn't the tool being over-eager; the
type really does leak through the public signature regardless of the
declaration. The real question is whether we *want* solr-core's public API to
expose caffeine, or whether that's accidental leakage worth encapsulating —
which is the judgment call, and why these are split out rather than applied
here.
##########
solr/core/build.gradle:
##########
@@ -121,40 +112,37 @@ dependencies {
testImplementation(libs.apache.curator.test, {
exclude group: 'org.apache.zookeeper', module: 'zookeeper'
})
- implementation(libs.apache.zookeeper.zookeeper, {
+ api(libs.apache.zookeeper.zookeeper, {
exclude group: "org.apache.yetus", module: "audience-annotations"
})
- implementation(libs.apache.zookeeper.jute) {
+ api(libs.apache.zookeeper.jute) {
Review Comment:
Reverted to `implementation` here (moved to #4614). For the record though,
this one is heavily exposed: ~82 public methods across core declare `throws
org.apache.zookeeper.KeeperException`, so ZooKeeper is genuinely part of the
public API surface today, not just embedded-ZK internals. Whether that's
desirable is #4614 discussion.
##########
solr/core/build.gradle:
##########
@@ -121,40 +112,37 @@ dependencies {
testImplementation(libs.apache.curator.test, {
exclude group: 'org.apache.zookeeper', module: 'zookeeper'
})
- implementation(libs.apache.zookeeper.zookeeper, {
+ api(libs.apache.zookeeper.zookeeper, {
exclude group: "org.apache.yetus", module: "audience-annotations"
})
- implementation(libs.apache.zookeeper.jute) {
+ api(libs.apache.zookeeper.jute) {
exclude group: 'org.apache.yetus', module: 'audience-annotations'
}
- testImplementation variantOf(libs.apache.zookeeper.zookeeper) {classifier
'tests'}
// required for instantiating a Zookeeper server (for embedding ZK)
runtimeOnly libs.xerial.snappy.java
runtimeOnly libs.dropwizard.metrics.core
- implementation(libs.jayway.jsonpath, {
+ api(libs.jayway.jsonpath, {
Review Comment:
Reverted to `implementation` here. The ABI reason it was flagged: `public
static Configuration jsonPathConfiguration()` returns a `com.jayway.jsonpath`
type — a real (if probably unintended) public exposure, so a good candidate for
encapsulating rather than promoting. Parked in #4614.
##########
solr/core/build.gradle:
##########
@@ -121,40 +112,37 @@ dependencies {
testImplementation(libs.apache.curator.test, {
exclude group: 'org.apache.zookeeper', module: 'zookeeper'
})
- implementation(libs.apache.zookeeper.zookeeper, {
+ api(libs.apache.zookeeper.zookeeper, {
exclude group: "org.apache.yetus", module: "audience-annotations"
})
- implementation(libs.apache.zookeeper.jute) {
+ api(libs.apache.zookeeper.jute) {
exclude group: 'org.apache.yetus', module: 'audience-annotations'
}
- testImplementation variantOf(libs.apache.zookeeper.zookeeper) {classifier
'tests'}
// required for instantiating a Zookeeper server (for embedding ZK)
runtimeOnly libs.xerial.snappy.java
runtimeOnly libs.dropwizard.metrics.core
- implementation(libs.jayway.jsonpath, {
+ api(libs.jayway.jsonpath, {
exclude group: "net.minidev", module: "json-smart"
})
// StatsComponents percentiles
implementation libs.tdunning.tdigest
-
// OpenTelemetry
api libs.opentelemetry.api
implementation libs.opentelemetry.context
- implementation(libs.opentelemetry.exporter.prometheus) {
+ api(libs.opentelemetry.exporter.prometheus) {
Review Comment:
Agreed it's surprising — reverted here. It's flagged because
`FilterablePrometheusMetricReader extends
io.opentelemetry.exporter.prometheus.PrometheusMetricReader` (public class,
public supertype). So the exposure is real, but "should a core consumer see it"
is exactly the right question — #4614 territory, not this PR.
--
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]
