KAFKA-20032: KIP-1265 Automatic detection of internal API usage#21337
KAFKA-20032: KIP-1265 Automatic detection of internal API usage#21337ashwinpankaj wants to merge 42 commits into
Conversation
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
|
This PR is being marked as stale since it has not had any activity in 90 days. If you If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact). If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed. |
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
…anner, suppression Addresses four review comments on KIP-1265: 1. Borrow Hadoop's @InterfaceAudience model. Replaces @publicapi with @InterfaceAudience.{Public,Private} (nested annotations mirroring the existing InterfaceStability shape). Audience is orthogonal to stability; missing annotation defaults to Private. KafkaProducer migrated to @InterfaceAudience.Public. PublicApi.java deleted. 2. Replace .java import-regex external check with ASM bytecode scan. New BytecodeApiUsageScanner walks compiled .class files (loose, in directories, or in jars) and records every referenced org.apache.kafka.* type via ClassVisitor/MethodVisitor/FieldVisitor. Catches Java, Scala, Kotlin and fully-qualified usages uniformly. PublicApiChecker exposes checkBytecode(List<File>) returning a ScanResult; the source-regex code path is removed. KafkaInternalApiCheckerTask input renamed sourceDirs->classDirs (default build/classes) and depends on the project's classes task. Maven mojo renamed sourceDirectories ->classesDirectories (default ${project.build.outputDirectory}). 3. Add @SuppressKafkaInternalApiUsage("reason") for known/intentional exposures. Public annotation in clients/.../annotation, RUNTIME retention, applicable to {TYPE, METHOD, FIELD, CONSTRUCTOR}. Scanner buffers class/method header references so an annotation visited after the header still suppresses them. When a reference is suppressed the reason is captured and surfaced in the report (text + JSON + console) as a dedicated "Suppressions" section, and logged at INFO. Empty value() rendered as "(no reason given)". 4. Annotations are @documented for javadoc visibility. Also cleans up two pre-existing test-source breakages in PublicApiCheckerTest and KafkaPublicApiCheckerTaskTest that referenced non-existent symbols, so buildSrc tests compile. 9 new BytecodeApiUsageScannerTest cases cover empty input, public refs, internal refs, JDK-ref filtering, jar inputs, field-type refs, and class-level/method-level/reasonless suppression. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase-2 work on the public-API checker: 1. Annotate the :clients module with @InterfaceAudience.Public on every class that appears in the javadoc jar. Adopts Hadoop-style audience inheritance — direct annotation only on top-level classes; nested classes inherit unless they carry an explicit @InterfaceAudience.Private. 556 top-level files in clients/src/main/java touched. 2. Resolve two real public-API "leaks" surfaced by the new checker: - Promote o.a.k.common.config.types.Password to @public and add the types package to the clients javadoc include list (was excluded accidentally despite being the return type of AbstractConfig.getPassword). - Annotate SampledStat.Sample as @public — the protected nested class is returned by SampledStat.current()/oldest() and is on the subclass-extension contract. 3. Add an auditable escape hatch for cascade leaks that need a KIP to resolve cleanly. The existing @SuppressKafkaInternalApiUsage annotation (originally consumer-side) is now also honoured at class/method/ctor level on the producer side; suppressed cascade findings show up in a dedicated section of the report with their reason. Applied to 10 ctor leaks of internal o.a.k.common.utils.Time / ProduceRequestResult / CloseableVerificationKeyResolver. 4. Refactor PublicApiChecker with SOLID decomposition: - PublicApiChecker (facade, ~100 LOC): pre-scans jars into an ApiSurface and delegates to focused validators. - ApiSurface: immutable per-class facts + query API (factsOf / jarOf / isEffectivelyPublic / isDeprecated). Hides the binary-vs-dotted name distinction at the boundary. - ApiSurfaceScanner: two-pass ASM scan that resolves audience inheritance. - CascadeValidator: method-signature cascade with the @SuppressKafkaInternalApiUsage escape hatch. - JavadocConsistencyValidator: MISSING_JAVADOC / MISSING_PUBLICAPI_ANNOTATION cross-checks against the javadoc HTML. - ClassFacts: per-class bytecode facts (private fields, Builder). - CheckResult: (violations, suppressions) wrapper — all validators return this for uniform composition. PublicApiChecker no longer takes a ClassLoader; all bytecode reading is ASM-only, so no LinkageError from broken transitive deps. 5. Rename BytecodeApiUsageScanner -> PluginDeveloperApiUsageScanner to match its actual role. Verified: ./gradlew :clients:kafkaPublicApiChecker -> 0 violations, 10 suppressions (all auditable in the report). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ence.Public Annotate the 6 classes that appear in the tools-api javadoc jar: Decoder, DefaultDecoder, IntegerDecoder, LongDecoder, StringDecoder, RecordReader. All MISSING_PUBLICAPI_ANNOTATION; no cascade violations. Verified: ./gradlew :tools:tools-api:kafkaPublicApiChecker -> 0 violations, 0 suppressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-jar support Annotate the 15 top-level classes in :storage:storage-api with @InterfaceAudience.Public (2 nested classes — RemoteLogSegmentMetadata.CustomMetadata and RemoteStorageManager.IndexType — inherit via the Hadoop-style audience model). Hits a cross-module cascade gap: storage-api's public methods reference o.a.k.common.TopicIdPartition and o.a.k.common.Uuid (both @public in clients), but the checker's per-module scan didn't include clients' jar so it couldn't see those annotations and false-flagged 17 INVALID_PARAMETER_TYPE / INVALID_RETURN_TYPE violations. Fix: add a referenceJarFiles input to KafkaPublicApiCheckerTask + extension. ApiSurfaceScanner.scan(projectJars, referenceJars) merges reference classes into the surface so cross-module @public references resolve via the membership set, but they don't take part in this module's MISSING_JAVADOC or cascade iteration — each module checks its own surface. build.gradle wires referenceJarFiles to sibling Kafka project deps via artifactView with componentFilter=ProjectComponentIdentifier and LibraryElements=JAR attribute (so we get the assembled jar, not the classes-dir variant Gradle picks by default for compile-classpath). Verified: ./gradlew :storage:storage-api:kafkaPublicApiChecker -> 0 violations, 0 suppressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-api with @InterfaceAudience.Public Annotate the 10 classes in the o.a.k.coordinator.group.api.assignor package that appear in the group-coordinator-api javadoc jar: PartitionAssignor, ConsumerGroupPartitionAssignor, ShareGroupPartitionAssignor, GroupAssignment, GroupSpec, MemberAssignment, MemberSubscription, PartitionAssignorException, SubscribedTopicDescriber, SubscriptionType. All MISSING_PUBLICAPI_ANNOTATION; no cascade violations. Cross-module references to clients types (Uuid in PartitionAssignor.uniqueId indirectly) resolve via the reference-jar plumbing added in the previous commit. Verified: ./gradlew :group-coordinator:group-coordinator-api:kafkaPublicApiChecker -> 0 violations, 0 suppressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…faceAudience.Public Annotate the three connect docsJar modules: - :connect:api (65 top-level + 4 nested via inheritance) - :connect:mirror-client (9) - :connect:test-plugins (11) All MISSING_PUBLICAPI_ANNOTATION; no cascade violations. Cross-module references (e.g. to clients' RecordHeaders, ConfigDef, KafkaException) resolve cleanly via the reference-jar plumbing. Verified per-module: ./gradlew :connect:api:kafkaPublicApiChecker -> 0 violations ./gradlew :connect:mirror-client:kafkaPublicApiChecker -> 0 violations ./gradlew :connect:test-plugins:kafkaPublicApiChecker -> 0 violations Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…king deprecation Annotate the streams docsJar modules: - :streams (206 top-level @InterfaceAudience.Public; nested inherit) - :streams:test-utils (5 top-level) - :streams:streams-scala (already passing — no changes) Checker tweak: ApiSurface.isDeprecated(name) now walks the enclosing-class chain so a nested class of a @deprecated outer is itself out of scope on both validation sides. Mirrors the @InterfaceAudience inheritance model. Resolves false MISSING_PUBLICAPI_ANNOTATION violations for MockProcessorContext.CapturedForward / CapturedPunctuator (nested in the deprecated o.a.k.streams.processor.MockProcessorContext). 10 cascade leaks suppressed with @SuppressKafkaInternalApiUsage: - QueryableStoreType.create + 5 QueryableStoreTypes nested impls leak internal StateStoreProvider — pending KIP review. - KafkaStreams.<init> x3 leak Time for test injection — same pattern as the Metrics ctors suppressed in :clients. - MockProcessorContext.recordCollector leaks internal RecordCollector. Verified per-module: ./gradlew :streams:kafkaPublicApiChecker -> 0 violations / 9 suppressions ./gradlew :streams:streams-scala:kafkaPublicApiChecker -> 0 / 0 ./gradlew :streams:test-utils:kafkaPublicApiChecker -> 0 / 1 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ede50d3 to
a60a161
Compare
Four knobs were declared, defaulted, plumbed through the
extension/task/mojo, and exposed via setters — but nothing in the
@TaskAction or Mojo#execute ever read them. Removed:
- KafkaPublicApiCheckerExtension.enforceJavadocConsistency
KafkaPublicApiCheckerTask.enforceJavadocConsistency
(and the wiring in KafkaPublicApiCheckerPlugin + the
redundant `enforceJavadocConsistency = true` line in root
build.gradle). Javadoc consistency is now always enforced,
which was the de facto behavior — the knob never gated
anything.
- KafkaPublicApiCheckerExtension.includePackages
KafkaPublicApiCheckerExtension.excludePackages
Neither was read by PublicApiChecker; the excludePackages
default of ["org.apache.kafka.common.internals"] was inert.
- KafkaInternalApiCheckerMojo.kafkaVersion (Maven)
The auto-detect-then-log branch in getKafkaJarsFromDependencies
set the field but no caller of the mojo consumed it, so the
"Auto-detected Kafka version" INFO log was the only visible
effect. Removed the field, the setter, and the matching
<parameter>/<configuration> entries in plugin.xml.
Test assertions in KafkaPublicApiCheckerTaskTest that exercised the
removed Property<Boolean> have been deleted along with the API.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iagnostic
KafkaPublicApiCheckerTask checks two distinct preconditions back-to-back:
1. The javadoc jar must exist on disk (jarFile = getJavadocJarFile()).
2. The projectJarFiles configuration must contain at least one entry.
The error message for (2) used `jarFile.getAbsolutePath()` — the javadoc
jar path from (1) — so a misconfigured `kafkaPublicApiChecker.projectJarFiles`
surfaced as
Project JAR file not found: build/libs/kafka-foo-X.Y-javadoc.jar
pointing the user at a file that exists and isn't the actual problem.
Replaced with a message that names the empty configuration knob so the
user knows what to wire up. Nothing else reads this exception.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
KafkaInternalApiCheckerTask resolved the Kafka jars at execution time by
iterating project.getConfigurations() and filtering by groupId. Only
classDirs was declared as a task input, so a Kafka version bump on its
own — same classDirs content, same task config — did not re-trigger the
check. The @DisableCachingByDefault annotation masked the gap but didn't
fix it; the check could still come up as UP-TO-DATE against a stale
surface.
New shape:
- Extension and Task both expose a kafkaDependencyJars file collection.
On the Task it's annotated @InputFiles @PathSensitive(RELATIVE), so
Gradle treats jar-path/content changes as a real input change.
- The plugin's existing JavaPlugin reactive hook now also contributes
the org.apache.kafka-filtered ArtifactView of compileClasspath and
testCompileClasspath to extension.kafkaDependencyJars. ArtifactView
with a componentFilter on ModuleComponentIdentifier#group resolves
lazily at execution time, so configuration-cache compatibility is
preserved, but Gradle still records the resolved file set as the
task input.
- The @TaskAction now consumes kafkaDependencyJars.getFiles() instead
of iterating project.getConfigurations() at exec time. The
getKafkaJarsFromDependencies() method and its
Configuration/ResolvedArtifact imports are gone. Updated
@DisableCachingByDefault's `because` accordingly.
Users can extend with `kafkaDependencyJars.from(...)` or override with
`.setFrom(...)` for non-standard layouts.
Docs: refreshed the Gradle snippet in docs/apis/internal-api-checker.md.
The old snippet still said `classDirs = files('build/classes') //
default`, but classDirs has defaulted to sourceSets.main.output.classesDirs
since the earlier JavaPlugin reactive wiring. Restructured the doc to
state the defaults that the plugin derives from the `java` plugin
(classesDirs + the Kafka subset of compile/testCompile classpaths) and
moved the explicit knobs into an "override only when the defaults don't
fit" example, including the new kafkaDependencyJars knob.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
plugin.xml was hand-maintained with three sources of drift:
1. <version>1.0.0-SNAPSHOT</version> — frozen even though the Maven
publication's version now tracks the Kafka release via gradle.properties.
2. Four hardcoded Maven dependency <version> entries (3.9.4 / 3.9.0)
that duplicate the same coordinates already declared in the Gradle
dependency block.
3. Implicit assumption that anyone editing the dep block would also
remember to edit the descriptor.
Single source of truth:
- buildSrc/build.gradle hoists `mavenApiVersion` and
`mavenPluginAnnotationsVersion` into local variables that drive both
the dependency block and the descriptor.
- processResources runs Ant's ReplaceTokens filter over the descriptor
on its way into the jar, substituting @Version@,
@mavenApiVersion@, and @mavenPluginAnnotationsVersion@.
- Ant's @token@ delimiter is intentional: Maven's own runtime
substitutions inside the descriptor (e.g. ${project.build.directory},
${kafka.internal-api-checker.enabled}) use ${...}, so the @ delimiter
cannot clash with them.
Verified after build: the descriptor in build/resources/main/ reports
<version>4.4.0-SNAPSHOT</version>, the four Maven dep versions resolve
to 3.9.4 / 3.9.0, and every Maven ${...} expression is still present
verbatim.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The internal-api-checker Gradle plugin is published for plugin/connector
developers but Kafka's own build never applies it (only the
public-api-checker is wired in), so the consumer side of the checker —
plugin/task registration, the JavaPlugin reactive hook, the @TaskAction's
violation routing — had no coverage in :buildSrc:test. Any breakage there
would have shipped through CI silently.
Two new test classes:
- KafkaInternalApiCheckerPluginTest (ProjectBuilder)
* applying the plugin registers the extension, the
kafkaInternalApiChecker task, and the skipInternalApiCheck helper.
* applying it together with JavaPlugin contributes
sourceSets.main.output.classesDirs to extension.classDirs, wires
extension.kafkaDependencyJars, and adds kafkaInternalApiChecker to
`check`'s dependencies.
- KafkaInternalApiCheckerTaskTest (ProjectBuilder + ASM-synthesised
Kafka jar and consumer .class files; runs task.checkInternalApiUsage()
in-process)
* checkerEnabled=false → early return, no scan.
* empty kafkaDependencyJars → warns and returns, doesn't treat every
org.apache.kafka.* reference as a violation.
* consumer references an internal Kafka class with failOnViolation=true
→ GradleException whose message mentions "internal API usage
violations".
* same scenario with failOnViolation=false → warns, does not throw.
* consumer references the synthetic @InterfaceAudience.Public class
→ passes.
To let these tests live alongside the existing KafkaPublicApiCheckerTaskTest
in org.apache.kafka.gradle, the AsmClassFactory / TempJarBuilder helpers
in org.apache.kafka.publicapi were widened from package-private to public
(mechanical visibility change to test-only code).
:buildSrc:test now reports 93 passing tests (was 86), zero failures.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A grab-bag of small correctness and hygiene items from the review of the
buildSrc checker. No behaviour change for valid inputs.
ReasonCaptureVisitor extracted to a single package-private class.
Previously two byte-for-byte copies (one inside CascadeValidator, one
inside PluginDeveloperApiUsageScanner) shared the same logic for reading
@SuppressKafkaInternalApiUsage value() — and the same "annotation
present, value() omitted ⇒ empty reason" convention. The five usage sites
(cascade class/method/field, scanner class/field/method) now resolve to
the shared class.
JSON report writer hardened.
- The hand-rolled escapeJson previously handled only \\, \", \n, \r,
\t. A suppression reason with any other control character — easy to
hit because the reason is free-form user text — landed in the output
as a raw byte and broke the JSON document. Rewritten to use a switch
plus StringBuilder; handles the standard shorthand escapes (\\, \",
\b, \f, \n, \r, \t) and falls back to \uXXXX for any character below
U+0020.
- The .txt → .json path derivation, previously duplicated across
KafkaInternalApiCheckerTask, KafkaPublicApiCheckerTask, and
KafkaInternalApiCheckerMojo, used String.replace(".txt", ".json").
That replaces *every* occurrence: a report named
"my.txt.report.txt" became "my.json.report.json". Centralised in
ViolationReporter#jsonReportFor, which strips a trailing ".txt"
suffix only and otherwise appends ".json".
- Both report writers now use Files.newBufferedWriter(path, UTF_8)
instead of the default-charset FileWriter — eliminates the
mojibake-on-non-UTF-8-platform risk the review flagged.
Emojis removed from source literals at all 7 sites (✅ × 4 in
ViolationReporter / KafkaPublicApiCheckerTask / KafkaInternalApiCheckerTask /
KafkaInternalApiCheckerMojo, ❌, ⚠️ , ℹ). They carried no information the
adjacent text didn't already, and the reviewer flagged them as unusual
for ASF source. The console-color highlighting still distinguishes
success from failure visually.
ASM Type.getInternalName() guarded against undefined input. The five
call sites in PluginDeveloperApiUsageScanner (return + arg types in three
method visitors) now route through referenceInternalName(Type), which
returns the internal name only for OBJECT / ARRAY types and null for
VOID / primitives. getInternalName() happens to return "V", "I", etc. on
primitives today and the prefix-gate later rejects those, but the
behaviour is undocumented. Downstream callers already null-tolerate.
ApiSurface#normalize gained a comment explaining that the $ ↔ . swap is
a Java-source nesting convention. Scala/Kotlin compiler-synthetic names
(Foo$, Foo$$anonfun$1) get rewritten too, but those are not part of the
@InterfaceAudience.Public surface, so any normalization confusion stays
below the org.apache.kafka.* prefix gate.
Tests: new ViolationReporterTest (7 cases) covers escapeJson — null,
each standard shorthand, control chars rendered as \uXXXX, plain chars
left alone — and jsonReportFor's three edge cases (suffix swap,
intermediate-.txt non-mangling, append-when-no-suffix). :buildSrc:test
is now at 100 passing.
Out of scope for this commit (would be larger refactors per the review's
own framing): merging CheckResult / ScanResult, consolidating the four
enclosing-chain walks, and renaming the publicapi package.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CheckResult and ScanResult were near-identical: same two fields (the
violations list and the suppressions list), same intent, differing only
in cosmetics (record-style violations() vs JavaBean getViolations(), and
ScanResult null-coalescing its constructor inputs to empty lists). The
two-DTO setup forced the consumer-scanner path and the validator path
to keep parallel result types for no semantic reason.
Merged into CheckResult:
- ScanResult deleted.
- PluginDeveloperApiUsageScanner.scan and PublicApiChecker.checkBytecode
now return CheckResult.
- CheckResult gained the null-coalescing constructor and hasViolations()
convenience from ScanResult, so the safer constructor wins.
- All call sites (Mojo, KafkaInternalApiCheckerTask, three test files)
migrated from getViolations() / getSuppressions() to the record-style
violations() / suppressions() that CheckResult already exposed.
Consolidated the enclosing-chain walks (option A):
- PluginDeveloperApiUsageScanner.outermostOf moved to
ClassFacts.outermostBinaryName so the two binary-name string helpers
(parentBinaryName for stepping one level outward, outermostBinaryName
for the leftmost segment) live next to each other and ClassFacts'
existing enclosingName() uses them too.
- ApiSurface.isEffectivelyPublic and isDeprecated, previously two
near-identical walk loops, now delegate to one private
findInChain(name, Predicate<ClassFacts>) helper. Each retains its own
Public/Private and Deprecated predicate; the loop shape lives once.
ApiSurfaceScanner.resolveEffectiveAudience deliberately untouched. It
walks a binary-keyed map and stops only at top-level, whereas ApiSurface
walks a dotted-keyed map and stops at the first absent enclosing
intermediate. Unifying those two semantics (the review's Bug apache#5) requires
re-keying ApiSurface's map by binary name, which is a real refactor; the
review itself flagged the symptom as "Low practical impact" so it stays
out of this commit. The two helpers in ClassFacts now make a future
unification straightforward — both walks would call parentBinaryName.
:buildSrc:test still reports 100 passing, zero failures.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the JSON report path. The checker writes a text report (grouped by
violation type and by class, with a separate suppressions section) and
prints a colour-coded summary to the console; nothing inside this repo
or in any of the user-facing docs consumed the parallel JSON output.
Carrying a second, hand-rolled serializer adds maintenance load (the
escape function, the path-derivation helper, three task / mojo
duplicated call sites) without buying anything.
Removed:
- ViolationReporter.writeJsonReport (both overloads), writeJsonEntries,
escapeJson, jsonReportFor, plus the back-compat printToConsole
overload that no caller used.
- The "Also write JSON report" three-line block from
KafkaPublicApiCheckerTask, KafkaInternalApiCheckerTask, and
KafkaInternalApiCheckerMojo.
- ViolationReporterTest, whose every case exercised escapeJson or
jsonReportFor.
- "Each run writes both text and JSON reports under …{txt,json}"
in docs/apis/internal-api-checker.md, replaced with the text-only
description.
Rename org.apache.kafka.publicapi -> org.apache.kafka.apicheck. The
former name covered only the Kafka-internal validators
(CascadeValidator, JavadocConsistencyValidator) — but the same package
also houses the consumer-facing internal-usage scanner
(PluginDeveloperApiUsageScanner) and the shared surface model
(ApiSurface, ApiSurfaceScanner, ClassFacts, CheckResult,
PublicApiViolation, ReasonCaptureVisitor, ViolationReporter,
PublicApiChecker). "apicheck" captures both audiences without
overloading "public api".
git-mv-rename for 19 production + test files (the rename is the only
substantive change to those files — Git tracks the move). Cross-package
imports in KafkaPublicApiCheckerTask, KafkaInternalApiCheckerTask,
KafkaInternalApiCheckerMojo, and KafkaInternalApiCheckerTaskTest
updated to point at the new package; same for the prose mention in
buildSrc/README.md (including the layout diagram).
No public Maven coordinate changes: the package is buildSrc-internal,
visible only to the plugin / mojo entry points that already shipped
under org.apache.kafka.gradle and org.apache.kafka.maven.
:buildSrc:test still passes (93 cases, zero failures); root project
still configures.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ignors StickyAssignor and CooperativeStickyAssignor are @InterfaceAudience.Public classes; both override AbstractStickyAssignor#memberData(Subscription), whose return type is the internal MemberData DTO. The override signature is forced by the abstract parent (which lives in org.apache.kafka.clients.consumer.internals and has no audience annotation, so defaults to Private), so neither subclass can return a different type without restructuring the inheritance. Annotate each override with @SuppressKafkaInternalApiUsage carrying a "pending refactor" reason — same pattern the codebase already uses for the Time and CloseableVerificationKeyResolver leaks. The proper fix (promote MemberData to @InterfaceAudience.Public, or replace the hook with a public-typed equivalent) lives in a follow-up task; the suppression keeps the checker green and the report records the justification so reviewers see the escape hatch on every build. After this commit `./gradlew :clients:kafkaPublicApiChecker` reports 0 violations / 12 suppressions (was 2 / 10). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…new field check The Bug apache#2 fix (CascadeValidator now visits fields and widens the gate from public-only to public+protected) caught 15 real cascade leaks in the @public Streams and MirrorMaker classes: Streams (9): - AutoOffsetReset.offsetResetStrategy (protected → StrategyType) - KafkaStreams.{streamsMetadataState, topologyMetadata, stateDirectory} (protected → StreamsMetadataState / TopologyMetadata / StateDirectory) - KafkaStreams(TopologyMetadata, ...) protected ctor - StreamsBuilder.{internalTopologyBuilder, internalStreamsBuilder} (protected → InternalTopologyBuilder / InternalStreamsBuilder) - Topology.internalTopologyBuilder (protected → InternalTopologyBuilder) - Topology(InternalTopologyBuilder) protected ctor MirrorMaker (6): - Checkpoint.{VALUE_SCHEMA_V0, KEY_SCHEMA, HEADER_SCHEMA} (public static final → o.a.k.common.protocol.types.Schema) - Heartbeat.{VALUE_SCHEMA_V0, KEY_SCHEMA, HEADER_SCHEMA} (same) All sites are pre-existing intentional extension points or historical wire-format constants; refactoring them properly (promoting the type or restructuring the API) needs a follow-up KIP. Annotate each with @SuppressKafkaInternalApiUsage carrying a KIP-1265 reason — same "pending KIP review" pattern already used in this PR for Time, CloseableVerificationKeyResolver, StateStoreProvider, and the sticky assignors' MemberData override. After this commit `./gradlew docsJar` is green across all 10 docsJar modules. Per-module suppression counts move: :clients 10 → 12 (sticky assignors' MemberData ×2) :streams 9 → 18 (the 9 above) :connect:mirror-client 0 → 6 (the 6 above) Other modules unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n types KAFKA-20626 (apache#22636) landed on trunk after this branch was last in sync. It added two new admin-client types that surface in clients javadoc: - StreamsGroupTopologyDescription (with nested Subtopology, Node, Source, Processor, Sink, GlobalStore) - StreamsGroupTopologyDescriptionStatus Without @InterfaceAudience.Public on either outer, the KIP-1265 checker surfaced eight MISSING_PUBLICAPI_ANNOTATION findings (the two outers and the six nested classes inherited through them) plus two cascade leaks on StreamsGroupDescription whose ctor parameter and return type referenced the unannotated Status. Annotated both outer classes; nested classes inherit @public per the KIP's Hadoop-style audience-inheritance rule (so the six nested findings clear together with the outer), and the cascade findings clear because Status is now part of the public surface. `./gradlew kafkaPublicApiChecker` across every module is now BUILD SUCCESSFUL: 0 real violations, 37 suppressions repo-wide. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
omkreddy
left a comment
There was a problem hiding this comment.
Can we use a module name such as kafka-build-plugins or kafka-api-checker instead of buildSrc?
| | Plugin / Mojo | ID / artifactId | Audience | | ||
| |---|---|---| | ||
| | `KafkaPublicApiCheckerPlugin` | `org.apache.kafka.public-api-checker` (Gradle) | Internal — applied to Kafka's own build to validate that `@InterfaceAudience.Public` types only expose other public types, and that javadoc inclusion matches the audience annotations. | | ||
| | `KafkaInternalApiCheckerPlugin` | `org.apache.kafka.internal-api-checker` (Gradle) | External — published for plugin/connector/Streams-app developers to detect references from their bytecode to non-`@Public` Kafka classes. | |
There was a problem hiding this comment.
Can we use the id as kafka-internal-api-checker-gradle-plugin' which will be similar to kafka-internal-api-checker-maven-plugin
| # The version is read from gradle.properties — release.py has already bumped it via | ||
| # `updateVersion` above — so no extra -P flags are needed beyond the publish credentials, | ||
| # which buildSrc picks up from ~/.gradle/gradle.properties (mavenUrl/mavenUsername/mavenPassword). | ||
| cmd("Building and uploading archives", "./gradlew :buildSrc:publish -PscalaVersion=2.13", cwd=kafka_dir, env=jdk25_env, shell=True) |
There was a problem hiding this comment.
Can we confirm that this artifact uses the same version as the Kafka release? It also looks like the -PscalaVersion=2.13 flag is not required here.
…cope, no-deps fail mode, deprecation bypass) Four findings from the second-round PR review. apache#1 Generic signatures in CascadeValidator (Medium). The consumer scanner walked the generic signature via SignatureReader, but the cascade side only inspected the erased descriptor — so a public method like `Map<String, InternalFoo> getFoos()` slipped through (the erased return type is just `Map`). Asymmetric with the consumer side, and contrary to the KIP's "method signatures" wording. Wired a SignatureReader walk into both `visitMethod` and `visitField` in CascadeValidator, mirroring the consumer scanner's `collectSignatureRefs`. The new walk surfaced 5 real generic-only leaks in :streams, all suppressed with KIP-1265 reasons: - KafkaStreams.threads (List<StreamThread>) - KafkaStreams.processStreamThread(Consumer<StreamThread>) - KafkaStreams.allLocalStorePartitionLags(List<Task>) - UnlimitedWindows.windowsFor (Map<Long, UnlimitedWindow>) - TimeWindows.windowsFor (Map<Long, TimeWindow>) In every case the erased type is a benign collection; the internal type appears only in the generic signature. apache#5 Gradle / Maven scope alignment (Medium). The Gradle plugin scans `sourceSets.main.output.classesDirs` by default; the Maven mojo was scanning `main + test`. Test code legitimately uses internal/test utilities (TestUtils, kafka-clients:test), so the two paths produced different noise levels for the same project. Maven now defaults to main only; users wanting test coverage can set `<classesDirectories>` explicitly. apache#6 Silent pass on no Kafka dependency (Medium). Both the Gradle task and the Maven mojo previously warned and skipped when no `org.apache.kafka:*` artifact was on the classpath — a configuration mistake produced a "0 violations" report that looked clean. Added a `failOnNoKafkaDependency` knob (Gradle extension + task, Maven mojo + plugin.xml descriptor), defaulting to `false` for back-compat. When true, the missing-deps condition is a hard failure. The warning message names the knob so users know how to make it fatal. apache#3 Drop deprecation bypass on the consumer-side predicate (Low-Medium). `PublicApiChecker.isPublicApi` previously returned true for any `@Deprecated` Kafka class. The cascade validator has its own deprecation bypass on the producer-side leak check (that policy stays — a deprecated public method on its way out shouldn't be treated as a fresh leak). But on the consumer side, deprecated-internal types are exactly the highest- risk references: they're the most likely to be removed in the next release. The consumer scanner now flags them. Updated PublicApiCheckerTest's `isPublicApi_deprecated_*` case to assert the new policy. Repo-wide `./gradlew kafkaPublicApiChecker` is BUILD SUCCESSFUL after this commit: 0 violations, 42 suppressions across all modules (was 37). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…le included build Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…producible reports, surface unjustified suppressions Five small polish items from the review file's minor/polish section, batched as one commit because each is a few lines and they're orthogonal: - api-checker/core/build.gradle: comment on the ASM 9.6 dep noting that ASM versions cap the JDK class-file major version they parse (Java 21 / class-file 65 today). Bump in lockstep with the max compile JDK or the scanner throws `Unsupported class file major version N` on fresh bytecode. - @InterfaceAudience.Public/.Private: added @target(ElementType.TYPE). All 600+ existing usages are type-level; the constraint matches reality and rejects future package/parameter/local misuse at compile time. (SuppressKafkaInternalApiUsage already carried the proper @target set.) - CheckResult constructor: wraps both lists in List.copyOf so the result is an immutable snapshot; null-handling preserved (List.of() fallback) so the "either side may be omitted" contract still holds. - ViolationReporter: report contents are now reproducible. Dropped the LocalDateTime.now() header, replaced HashMap-order groupingBy with TreeMap-keyed grouping, and added a stable VIOLATION_ORDER comparator (className → memberName → description) used for both the grouped sections and the suppressions list. Two runs over the same inputs now produce byte-identical reports. - Surface unjustified suppressions: added PublicApiViolation.NO_REASON_MARKER as the single source of truth for the "(no reason given)" string, plus a PublicApiViolation#lacksReason() helper. The two sites that rendered empty reasons (CascadeValidator#asSuppression and PluginDeveloperApiUsageScanner) now reference the constant. All three task/mojo summaries (KafkaPublicApiCheckerTask, KafkaInternalApiCheckerTask, KafkaInternalApiCheckerMojo) count reason-less suppressions and log a WARN with the count and a pointer to KIP-1265's "reason required" guidance. `./gradlew :api-checker:core:test :api-checker:gradle-plugins:test` still green. `./gradlew :clients:kafkaPublicApiChecker` confirms the new report layout has no timestamp and the suppressions are sorted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
) + group cascade classes by jar Two polish items from the review's minor section. ApiSurface.findInChain (used by isEffectivelyPublic and isDeprecated) and ApiSurfaceScanner.resolveEffectiveAudience used to disagree on missing intermediates. The scanner walked by lastIndexOf('$') and kept going past gaps; the surface walked by factsOf().enclosingName() and stopped at the first absent link. For a type nested under a synthetic enclosing class that isSyntheticOrAnonymous filters out (e.g. Outer$1$Inner) the scanner would resolve to Outer's audience while the surface returned the @Private default — quietly inconsistent. Both walks now share the rule: when the current level resolves to ClassFacts, step via ClassFacts#enclosingName (preserves dotted-form input from JavadocConsistencyValidator); when it doesn't, fall back to ClassFacts#parentBinaryName so a missing intermediate doesn't end the chain. Added regression test isEffectivelyPublic_skipsMissingIntermediates. CascadeValidator was opening the project jar once per externally-visible @public class — O(classes) JarFile constructions for a single jar. The validator now groups effectively-public classes by their source jar in a LinkedHashMap, opens each jar once, and reuses the open JarFile for every class.checkClass call. Pulled the outer try-with-resources out of checkClass; the per-class loop sits inside the per-jar try. Repo-wide ./gradlew kafkaPublicApiChecker still green; api-checker/core tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… doc gaps, test coverage - CascadeValidator: cover class-header extends/implements + generic supertype signatures so a @public class can't silently inherit from an internal type; skip package-private supertypes a consumer can't name. Refactor inner ClassVisitor into a named nested class with extracted BufferedMember/Field visitors so the duplicated suppression-flush logic lives in one place. - Scanner: walk try-catch on internal exception types; document the four reference kinds the bytecode scanner intentionally does not follow (parameter annotations, type-use annotations, annotation-only class literals, inlined compile-time constants). - Gradle plugins: replace doLast-mutation skip tasks with a configure-time -PkafkaPublicApiChecker.skip / -PkafkaInternalApiChecker.skip property gate, plus user-facing docs section. - Suppressions surfaced by the new supertype check: StickyAssignor, CooperativeStickyAssignor, RangeAssignor, RoundRobinAssignor, JwtRetriever, JwtValidator, TimeWindows, UnlimitedWindows, TimeWindowedSerializer, SessionWindowedSerializer, MockProcessorContext. - Tests: header-buffer (class-level suppression diverts extends/implements findings), abstract-method flush, signature-walk coverage, plus the skip-property path on the Gradle plugin. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ectExistingRoots, split NPath helpers The checker no longer lives in buildSrc, so update the comments and the checkstyle import-control filename to reflect the api-checker included build. - Rename checkstyle/import-control-buildsrc.xml → import-control-api-checker.xml and update its internal comment + the configProperties reference in api-checker/build.gradle. - Replace "buildSrc classpath" framing in three comment blocks (api-checker README, settings.gradle, api-checker/build.gradle) with "main-build classpath". - Extract handleNoKafkaDependency / runCheck / reportResults helpers in both KafkaInternalApiCheckerMojo and KafkaInternalApiCheckerTask to land NPath complexity below the 500-branch checkstyle ceiling. - Lift collectExistingRoots into PublicApiChecker.collectExistingRoots so the two adapters share a single implementation. - Replace .toLowerCase() in KafkaPublicApiCheckerPlugin.isPublicApi with equalsIgnoreCase to satisfy the Locale-safety regexp check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… 92 files These files were touched by an early formatting pass on this branch that collapsed blank lines between the import block and the type declaration. Diff vs trunk for every file in this commit is empty when blank lines are ignored, i.e. they carry no real KIP-1265 change. Restore them to trunk to keep the PR scoped to the actual KIP-1265 work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Wires up the KIP-1265 mechanism for explicit declaration and automated
enforcement of Apache Kafka's public API surface. Adds two annotations
(
@InterfaceAudience.Public/.Private, modelled on Hadoop andorthogonal to the existing
@InterfaceStability), an auditable escapehatch (
@SuppressKafkaInternalApiUsage), a Gradle checker thatcross-validates the javadoc HTML against the bytecode annotations and
cascades through public method signatures, and a Gradle/Maven checker
plugin for external plugin developers. Every javadoc'd class across the
10 docsJar modules is annotated, and
./gradlew docsJarnow finalizeswith the checker on every project — 0 violations, 20 suppressions, every
suppression carrying a reason the next reviewer can audit.
JIRA: KAFKA-20032 ·
KIP:
KIP-1265
What's in this PR
Annotations (in
o.a.k.common.annotation)@InterfaceAudience.Public/.Private— marker annotations, RUNTIMEretention,
@Documented. The audience dimension is orthogonal to@InterfaceStability; a class may carry both.@SuppressKafkaInternalApiUsage("reason")—class/method/field/constructor target, RUNTIME retention. Required
reason text is surfaced in every checker report so each escape hatch is
reviewable inline.
Audience inheritance
A class's effective audience is its own direct
@InterfaceAudienceannotation if present, otherwise the audience of its nearest annotated
enclosing class. Default everywhere is
@Privateper the KIP. A nestedclass can override an inherited
@Publicwith explicit@InterfaceAudience.Private. This matches Hadoop's model and alignswith how javadoc treats nested classes — they're documented under the
outer's page, so requiring a separate annotation on every public nested
class would be redundant noise. Inheritance also handles protected and
package-private nested classes correctly: they appear in javadoc only as
part of the outer's docs, so the checker only enforces direct
@Publicfor the
MISSING_JAVADOCcheck, while cascade reference checks acceptinherited
@Publicfor any nested type. Net effect: ~640 javadoc'dclasses are covered by ~550 top-level annotations, with the rest carried
by inheritance.
Producer-side checker (
kafkaPublicApiChecker— runs as part ofdocsJar)Cross-validates each module's javadoc jar against its bytecode-level
annotations. Reports three categories of finding:
MISSING_PUBLICAPI_ANNOTATION— a class is in the javadoc HTML butisn't effectively
@Public.MISSING_JAVADOC— a class carries a direct@Publicbut no HTMLpage in the javadoc jar.
INVALID_RETURN_TYPE/INVALID_PARAMETER_TYPE/INVALID_EXCEPTION_TYPE) — a public method signature on a@Publicclass leaks an internal type. Cascade findings whose containing method
or class carries
@SuppressKafkaInternalApiUsagedivert to a separatesuppressions section with the supplied reason.
Consumer-side checker (
kafkaInternalApiCheckerplugin)Gradle and Maven plugins
(
o.a.k:kafka-internal-api-checker-maven-plugin) for external plugindevelopers — walk compiled bytecode (Java/Scala/Kotlin, uniformly) and
flag references to any
org.apache.kafka.*type that isn't@Public.The same
@SuppressKafkaInternalApiUsageannotation applies toknown/intentional consumer-side exposures.
Cross-module support
Per-module checks include sibling Kafka jars as
referenceJarFiles—reference classes contribute to the surface so cross-module
@Publicresolution works (e.g.
:storage:storage-apireferencingo.a.k.common.TopicIdPartitionfrom:clients), but they don'tparticipate in the module's own
MISSING_JAVADOCor cascade iteration.Gradle wires this via
configurations.compileClasspath.incoming.artifactView { componentFilter { it instanceof ProjectComponentIdentifier } … }filtered to the JARartifact variant.
Coverage
./gradlew docsJartriggers the checker on every subproject. Per-moduleresults across the 10 docsJar modules:
| Module | Violations | Suppressions | |---|---:|---:| |
:clients|0 | 10 | |
:streams| 0 | 9 | |:streams:test-utils| 0 | 1 | |:streams:streams-scala| 0 | 0 | |:connect:api| 0 | 0 | |:connect:mirror-client| 0 | 0 | |:connect:test-plugins| 0 | 0 ||
:storage:storage-api| 0 | 0 | |:tools:tools-api| 0 | 0 | |:group-coordinator:group-coordinator-api| 0 | 0 | | Total |0 | 20 |
Suppressions in this PR
Every suppression carries a
KIP-1265: …reason in the source and isrendered in the checker report. They mark cascade leaks that can't be
cleanly resolved without API-shape changes the KIP scope doesn't cover:
:clients(10) —Metrics(Time)× 6,KafkaMetric(Time),JwtBearerJwtRetriever(Time),ClientCredentialsJwtRetriever(Time),DefaultJwtValidator(CloseableVerificationKeyResolver). All cases ofinternal
o.a.k.common.utils.Time(or other internal-package types)reaching the public ctor surface for test-injection purposes.
:streams(9) —QueryableStoreType.create(StateStoreProvider)plus the five
QueryableStoreTypesnested overrides;KafkaStreams.<init>(…, Time, …)× 3 (same Time-injection pattern asMetrics).:streams:test-utils(1) —MockProcessorContext.recordCollector()returns internalRecordCollector.These are deliberate, reviewable, and easy to grep. Follow-up KIPs can
decide per-case whether to promote the internal type or refactor the
method signature.
Genuine public-API leaks surfaced and fixed
The checker found two cases where the de-facto public surface didn't
match the published javadoc:
o.a.k.common.config.types.Password— alreadypublic class,returned by
AbstractConfig.getPassword()which is@Public, but the…common.config.types.*package was missing from the:clientsjavadocinclude list. Brought it into the include list and annotated
Passwordwith
@Public.SampledStat.Sample—protected static classreturned by twopublic methods (
current(long),oldest(long)) of@Public abstract SampledStat. Subclasses extendingSampledStathave always been ableto touch this type; the annotation now makes the de-facto contract
explicit. Visibility unchanged.
Implementation notes
PublicApiChecker(~100 LOC) — facade. Constructor takes(List<File> projectJars, List<File> referenceJars)and pre-scans into an immutableApiSurface. Subsequent calls (checkPublicApiConsistency,isPublicApi,checkBytecode) are thin delegations.ApiSurface— immutable scan result. Hides the dotted-vs-binary namedistinction at the boundary (all lookups accept either form and
normalize internally).
ApiSurfaceScanner— two-pass staticscan(projectJars, referenceJars) → ApiSurface. Pass 1 reads bytecode facts per class viaASM; pass 2 walks the enclosing-class chain to resolve effective
audience.
CascadeValidator— method-signature cascade +@SuppressKafkaInternalApiUsagedetection.JavadocConsistencyValidator— javadoc HTML reader +MISSING_JAVADOC/MISSING_PUBLICAPI_ANNOTATIONcross-checks.ClassFacts— per-class bytecode facts (immutable, builder patternfor the ASM visitor).
PluginDeveloperApiUsageScanner— consumer-side ASM walker.No classloading anywhere. All bytecode reading goes through ASM, so
a class with broken transitive deps on the checker's classpath (gRPC
stubs, telemetry shims, etc.) can't trip
LinkageError/NoClassDefFoundError.Test plan
./gradlew docsJar— every module'skafkaPublicApiCheckerrunsand reports 0 violations.
suppressions across
:clients(10),:streams(9),:streams:test-utils(1).:buildSrc:compileJava+:buildSrc:compileTestJavaclean.referencing
KafkaProducer,ProducerRecord,Callback,RecordMetadata,StringSerializer) produce the expected violationsand suppressions.
Reviewers: Manikumar Reddy manikumar.reddy@gmail.com