SOLR-18302: DAGP external-lib api advice — ILLUSTRATIVE, not for merge (part 3/3)#4614
Closed
serhiy-bzhezytskyy wants to merge 8 commits into
Closed
SOLR-18302: DAGP external-lib api advice — ILLUSTRATIVE, not for merge (part 3/3)#4614serhiy-bzhezytskyy wants to merge 8 commits into
serhiy-bzhezytskyy wants to merge 8 commits into
Conversation
Completes the dependency-analysis follow-up deferred from the Gradle 9 upgrade (SOLR-18289), which dropped the Gradle-9-incompatible ca.cutterslade.analyze plugin. Adds the Dependency Analysis Gradle Plugin (com.autonomousapps build-health) configured to REPORT (warn), not fail — its advice is guidance for human review, not enforcement. Applies only the uncontroversial advice: - removal of genuinely-unused dependency declarations - a few test-scope corrections (implementation -> testImplementation) Deliberately NOT included here (left for separate case-by-case review): - implementation -> api promotions (DAGP is aggressive here; per dev@ these need human judgement, as liberal api is viral/transitive) Also forces kotlin-metadata-jvm to match Solr's Kotlin so DAGP can analyze the :solr:ui (Compose/KMP) module. Lockfiles regenerated.
github-actions
Bot
added
documentation
…oss-dc-manager tests) DAGP flagged the kafka-clients test-classifier jar as an unused dependency, so the earlier commit removed it. That was a false positive: no test compiles against it, but EmbeddedKafkaCluster (from the kafka-streams test jar) loads org.apache.kafka.test.TestCondition from it at runtime, so the *IntegrationTest suites failed with ClassNotFoundException on CI. Re-add it as testRuntimeOnly (runtime-only is the accurate scope, vs the testImplementation it originally had) with a comment, and restore its license sha1. A concrete example of why DAGP runs report-not-fail and its advice needs case-by-case judgement.
serhiy-bzhezytskyy
changed the title
…nts, fix changelog Responding to review feedback on the safe-advice PR: - Revert all external implementation->api promotions that had leaked in from the api-scope work (caffeine, zookeeper, jute, jsonpath, opentelemetry-sdk + exporter-prometheus in core; curator-client/jute in solrj-zookeeper; zookeeper +tests in test-framework; calcite in sql; cloud-storage in gcs; awssdk-s3 in s3). None are in the modules' public ABI and DAGP does not advise them; they belong in the separate api-scope discussion PRs, not here. Verified no downstream module relies on the former transitive leak. - Revert solrj-jetty entirely: its changes were api promotions + manual cleanups, none part of the safe mechanical advice. - Restore opentelemetry-sdk-metrics in test-framework: DAGP flagged it unused, but JettySolrRunner uses MetricReader from it and the prometheus exporter is declared transitive=false, so it is required (compile break otherwise). - Restore the gjf-SOLR-18296 changelog entry that had been deleted by a rebase artifact; it belongs to the separate google-java-format change. - Remove code comments that narrated the change/tool advice (per AGENTS.md); keep only terse why-comments matching existing style. - Changelog: type changed->other; link to SOLR-18302 (the correct JIRA). - Regenerate lockfiles/licenses.
…jetty
Remove testImplementation project(':solr:solrj-jetty') (a module test-depending on
itself — the test source set already sees its own main classes) and the duplicate
testImplementation project(':solr:solrj') (already api project(':solr:solrj')). The
project(path: ':solr:solrj', configuration: 'testArtifacts') declaration stays; it
provides solrj's test classes, which is a real dependency. Verified :solr:solrj-jetty
test compiles and all 59 tests pass.
serhiy-bzhezytskyy
force-pushed
the
dagp-pr3-external-api
branch
from
fbe4f31 to
6880181
Compare
…, doc the lockfile filter
- Convert gradle/validation/dependency-analyze.gradle to Kotlin DSL (.gradle.kts),
per the project's preference for Kotlin build files. Behavior is unchanged;
buildHealth runs and all suppressions (:solr:ui, solrj-jetty, jwt-auth,
report-not-fail) remain active.
- Remove the redundant self-referential testImplementation project(':self')
declarations in api, solrj, solrj-streaming, solrj-zookeeper, and core (two were
left-over 'cutterslade requires we state the obvious' workarounds for the plugin
removed in SOLR-18289). Test source sets already see their own main classes.
- Add a one-line comment explaining why resolveAndLockAll skips the plugin's
synthetic dagp.* configurations.
- Regenerate the changelog entry with the writeChangelog task so it matches the
documented format.
Applies DAGP's api advice ONLY for inter-module project() dependencies that are genuinely part of the consuming module's public API (e.g. solr-core's public types expose solrj), where 'api' is correct so downstream modules compile. External-library api promotions are intentionally excluded here (see part 3/3); per dev@ feedback those need case-by-case judgement rather than blanket promotion. Builds on apache#4612. Lockfiles regenerated.
…ions (part 3/3) Applies the 90 external-library implementation->api promotions DAGP advises across the modules, for discussion. Verified all 90 against each module's ABI dump: every one is genuinely exposed in public API (return type, parameter, field, or supertype) today, regardless of the declaration -- e.g. CaffeineCache implements caffeine's RemovalListener; ~82 core methods throw ZooKeeper's KeeperException. So this is an api-hygiene question (is the exposure intended, or worth encapsulating?), not the tool over-reaching. jersey-core-server is left as implementation: DAGP advises api (it is ABI-exposed), but its version comes from the implementation-scoped jersey BOM, so as api it fails to resolve. A concrete case of advice that is correct about exposure yet cannot be applied mechanically. Builds on apache#4613. check -x test passes; lockfiles regenerated.
serhiy-bzhezytskyy
force-pushed
the
dagp-pr3-external-api
branch
from
6880181 to
57c861b
Compare
Contributor
Author
|
Closing per discussion on #4612 — this was always illustrative (the full external-lib api advice for discussion), not for merge. The api-vs-implementation calls are best made case-by-case, not applied wholesale. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Draft — NOT for merge. Part 3 of 3, for discussion. Builds on #4613 → #4612.
This applies the remaining ~89 external-library
implementation → apipromotions that DAGP advises across the modules, so the full picture is visible as code.Correction to my earlier framing (dev@ thread): I originally described these as the tool being "api-happy." Having actually checked, that was wrong, and I want to be straight about it. I verified every one of the 90 advised external promotions against each module's ABI dump (resolving the real jar packages so coordinate↔package mismatches couldn't hide anything). All 90 are genuinely ABI-exposed — the library type appears in a public return type, parameter, field, or supertype today, regardless of the
implementation/apideclaration. Examples:caffeine—CaffeineCache implements com.github.benmanes.caffeine.cache.RemovalListenerzookeeper— ~82 public methods declarethrows org.apache.zookeeper.KeeperExceptionjsonpath—public static Configuration jsonPathConfiguration()simplemagic—FileTypeMagicUtil implements ContentInfoUtil.ErrorCallBackkafka-clients(cross-dc) —MirroredSolrRequestSerializer implements kafka SerializerSo this isn't "the tool over-reaching." The real question is one of API hygiene: do we want these types to be part of each module's public contract, or is the exposure accidental and worth encapsulating (narrowing the signatures) instead of promoting? That's the case-by-case judgement, and it's exactly why this is a discussion PR rather than something to merge.
One promotion is genuinely not applicable: DAGP advises
jersey-core-server→apiin:solr:core(it is ABI-exposed), but promoting it breaks version resolution — its version comes from the jersey BOM, which is declaredimplementation, so asapiit resolves to no version. Left asimplementationhere. A real example of advice that's correct about exposure yet can't be applied mechanically.Suggested use: rather than accept/reject wholesale, point at any deps whose exposure is intended public API and I'll fold just those into #4613; for the rest, the better fix is likely encapsulation, tracked separately.
Layering: #4612 (safe advice, ready) → #4613 (inter-module project() api, all ABI-backed, draft) → this (external api, draft/illustrative).
AI-assisted, human-reviewed and verified per AGENTS.md.