KIP-1320 (PoC): deprecation approach for internal classes exposed in public packages#22758
Draft
unknowntpo wants to merge 2 commits into
Draft
KIP-1320 (PoC): deprecation approach for internal classes exposed in public packages#22758unknowntpo wants to merge 2 commits into
unknowntpo wants to merge 2 commits into
Conversation
2c1eafd to
c3f6796
Compare
…ecated shim common.utils.ByteBufferInputStream (never public API, but importable) is moved to common.utils.internals.ByteBufferInputStream, and the public class becomes a @deprecated(since="4.4", forRemoval=true) wrapper that extends the internal class (no copied code). Main-source and test callers switch to the internal class, and the class's test moves to the internals package. Main compiles clean under -Xlint:all -Werror. Discussion: https://lists.apache.org/thread/5hf11khclhngftp297hhvwc23j5bcx28
…ernals move A class with only a few internal callers can be deprecated in place instead of moved. common.utils.ProducerIdAndEpoch is marked plain @deprecated(since="4.4"), not forRemoval, since it stays for Kafka's internal use and its removal is not committed. Its 9 main-source callers add @SuppressWarnings("deprecation"), scoped to the methods in the large public classes (KafkaProducer, KafkaAdminClient) and at class level where usage is in many places (TransactionManager). Discussion: https://lists.apache.org/thread/5hf11khclhngftp297hhvwc23j5bcx28
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
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 / proof-of-concept for KIP-1320. Not meant to be merged as-is.
It shows the KIP's deprecation approach on a couple of small classes. It
is only a demonstration — the choice for any specific class is left to
the maintainers.
The idea
KIP-1320 deprecates classes that were never public API but sit in a
public package, so third parties can import them. Deprecating them gives
those users a compile-time warning before support ends at 5.0.0. There
are three cases; the first two involve code and each has an example
below:
1. Move to an internals package —
ByteBufferInputStreamByteBufferInputStreamis an internal-only utility, so it is moved tothe
internalspackage where it belongs(
org.apache.kafka.common.utils.internals.ByteBufferInputStream);@Deprecated(since = "4.4", forRemoval = true)wrapper thatextendsthe internal class, so nocode is copied. Existing
new ByteBufferInputStream(...)still works(source- and binary-compatible) and now shows a removal warning;
forRemoval = trueis correct here, because the public wrapper isreally removed in 5.0.0.
2. Deprecate in place —
ProducerIdAndEpochProducerIdAndEpochis used here only as an example of this case; it issimply marked
@Deprecated(since = "4.4")where it already is;@SuppressWarnings("deprecation"),since there is no internal class to switch to. In the large public
classes (
KafkaProducer,KafkaAdminClient) the annotation is put onlyon the methods that use it; class level is used only where a class uses
it in many places (for example
TransactionManager);@Deprecated(and the@SuppressWarningson thecallers) is removed. The class stays for Kafka's internal use, and from
5.0.0 the policy already treats it as unsupported, so the warning is no
longer needed.
3. Policy only, no
@Deprecated-- for rarely-used classesIf almost no one imports a class, a warning helps no one, and adding
@Deprecatedstill costs the caller changes shown above -- so it is notworth it. These classes are simply left to the policy (unsupported from
5.0.0), with no code change at all. For example,
LoggingSignalHandlerhas only about 5 external imports on GitHub, so it would get nothing but
the policy. There is no diff for this case, which is the point.
Which approach fits which class is up to the maintainers; this PoC shows
that the two code cases compile and work.
How it was checked
./gradlew compileJava(all modules,-Werror) — passescheckstyleMain,spotlessCheck— passByteBufferInputStreamcaller back to thedeprecated public class, the build fails with
error: warnings found and -Werror specified. This shows the callers really do need to be handled.Notes: for
ByteBufferInputStream, the test callers are moved to theinternal class too, and its own test moves to the internals package.
(Test compilation does not use
-Werror, so this was not strictlyrequired, but it keeps the code consistent.) Scala callers are not
affected because scalac runs with
-deprecation:false.Links
https://lists.apache.org/thread/5hf11khclhngftp297hhvwc23j5bcx28