KAFKA-20128: Add missing TimestampType to java doc and move non-public classes to internal subpackage

[lianetm]

• Add TimestampType to the java doc, as it is a public API introduced as
  part of KIP-32 (for ConsumerRecord) and KIP-65 (for SinkRecord).
• Move all non-public classes to an "internal" subpackage

Reviewers: Andrew Schofield aschofield@confluent.io, PoAn Yang
payang@apache.org, Maros Orsak maros.orsak159@gmail.com, Chia-Ping
Tsai chia7712@gmail.com

[lianetm]

This commit exposed it as part of the KIP implementation: 45c8195

With the PR, the API shows in the java docs as expected

[chia7712]

ugh, TimestampType is in a non-public package ... Maybe we need a KIP to create a "public" TimestampType and deprecate old one

[lianetm]

Agree, but just to make sure I'm following. The fact that this TimestampType is public makes the package public already, correct? we can already see the package exposed in the java docs for ConsumerRecord, but without link (this is why I found this btw) https://kafka.apache.org/40/javadoc/org/apache/kafka/clients/consumer/ConsumerRecord.html

That being said, totally agree that the situation of this package is a mix of private and public components. As alternative to the KIP path, could we also consider leaving the public TimestampType where it is (class and package are public),and move all the private classes instead? (to an "internal" folder maybe, that wouldn't be part of the public API) We have a similar structure for the consumer, this https://github.com/apache/kafka/tree/trunk/clients/src/main/java/org/apache/kafka/clients/consumer
wdyt?

[chia7712]

could we also consider leaving the public TimestampType where it is (class and package are public),and move all the private classes instead? (to an "internal" folder maybe, that wouldn't be part of the public API) We have a similar structure for the consumer, this https://github.com/apache/kafka/tree/trunk/clients/src/main/java/org/apache/kafka/clients/consumer

That is a practical idea to avoid a KIP. My concern, however, is that it leaves org.apache.kafka.common.record with only a single class. That feels a bit odd for a public package structure

[AndrewJSchofield]

could we also consider leaving the public TimestampType where it is (class and package are public),and move all the private classes instead? (to an "internal" folder maybe, that wouldn't be part of the public API) We have a similar structure for the consumer, this https://github.com/apache/kafka/tree/trunk/clients/src/main/java/org/apache/kafka/clients/consumer

That is a practical idea to avoid a KIP. My concern, however, is that it leaves org.apache.kafka.common.record with only a single class. That feels a bit odd for a public package structure

True, but this stuff is all a bit of a mess to be honest. That doesn't seem too bad to me.

[chia7712]

True, but this stuff is all a bit of a mess to be honest. That doesn't seem too bad to me.

2 > 1 😄

@lianetm do you plan to address it in this PR?

[lianetm]

Hi both, addressed the move here since it's a mechanical refactoring.

• left the public class TimestampType in the same package where it was
• added the package and the class to the java docs (they were already public)
• moved all other classes to an "internal" sub-package (which remains private) - this took updating all imports/references to them, so tons of files but just a mechanical refactor
  I call out below the changes introduced that are not related to the mechanical refactor. Let's see the build.

[lianetm]

this can now include all classes under the record package (it's only TimestampType for now)

[lianetm]

updated info for the package that was public with TimestampType

[lianetm]

same private-package description we had, but now at the "internal" subpackage level

[chia7712]

@lianetm could you update the stale comment?

  // When updating lz4 make sure the compression levels in org.apache.kafka.common.record.CompressionType are still valid
  // https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/record/CompressionType.java#L73-L74
  // https://github.com/yawkat/lz4-java/blob/main/src/java/net/jpountz/lz4/LZ4Constants.java#L23-L24

https://github.com/apache/kafka/blob/trunk/gradle/dependencies.gradle#L109