KAFKA-12612: Remove `checksum` from ConsumerRecord/RecordMetadata for 3.0 #10470

ijuma · 2021-04-03T16:20:20Z

The methods have been deprecated since 0.11 without replacement since
message format 2 moved the checksum to the record batch (instead of the
record).

Unfortunately, we did not deprecate the constructors that take a checksum
(even though we intended to) so we cannot remove them. I have deprecated
them for removal in 4.0 and added a single non deprecated constructor to
ConsumerRecord and RecordMetadata that take all remaining parameters.
ConsumerRecord could do with one additional convenience constructor, but
that requires a KIP and hence should be done separately.

Also:

Removed ChecksumMessageFormatter, which is technically not public
API, but may have been used with the console consumer.
Updated all usages of ConsumerRecord/RecordMetadata constructors
to use the non deprecated ones.
Added tests for deprecated ConsumerRecord/RecordMetadata`
constructors.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

ijuma · 2021-04-03T16:20:47Z

Will update upgrade.html soon.

ijuma · 2021-04-03T18:30:27Z

Will update upgrade.html soon.

Done.

dajac · 2021-04-06T14:43:18Z

core/src/main/scala/kafka/tools/ConsoleConsumer.scala

@@ -614,18 +613,3 @@ class NoOpMessageFormatter extends MessageFormatter {
  def writeTo(consumerRecord: ConsumerRecord[Array[Byte], Array[Byte]], output: PrintStream): Unit = {}
 }

-class ChecksumMessageFormatter extends MessageFormatter {


I wonder if we should explicitly mention this one in the upgrade notes. It could be considered as part of the console-consumer tool which is part of our public API. It is in a gray zone. I am pretty sure that it is not used anymore but how knows... What's you take on this?

Good question, I was debating it myself. Technically, this is not public API since it's not mentioned in the public javadoc and it's in the core module (everything is private in the core module).

However, it is possible that people would pass this class when using the console consumer. The checksum value has never been correct for message format 2, which has been the default since 0.11, so I figured the chances that people actually used this semi public API is low. Seems like there isn't much harm in mentioning it in the upgrade notes though, so will add.

…ecated since 0.11. These has been deprecated since 0.11, but there are no replacements. Message format 2 moved the checksum to the record batch (instead of the record). clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java

chia7712

@ijuma thanks for great cleanup!

chia7712 · 2021-04-07T04:30:39Z

clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java

+     * @deprecated use constructor without `checksum` parameter.
+     */
+    @Deprecated
+    public RecordMetadata(TopicPartition topicPartition, long baseOffset, long relativeOffset, long timestamp,


Not sure whether we have to offer this constructor. We remove the checksum from all constructors of ConsumerRecord even though we don't add deprecation annotation to constructors before.

I probably need to add back some of the ConsumerRecord constructors since they were not previously deprecated.

chia7712 · 2021-04-07T04:31:22Z

clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java

-    public long checksum() {
-        if (checksum == null)
-            // The checksum is null only for message format v2 and above, which do not have a record-level checksum.
-            this.checksum = DefaultRecord.computePartialChecksum(timestamp, serializedKeySize, serializedValueSize);


Could you remove computePartialChecksum? It is unused now.

chia7712 · 2021-04-07T04:34:22Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java

@@ -1385,7 +1385,7 @@ private void handleOffsetOutOfRange(FetchPosition fetchPosition, TopicPartition
            byte[] valueByteArray = valueBytes == null ? null : Utils.toArray(valueBytes);
            V value = valueBytes == null ? null : this.valueDeserializer.deserialize(partition.topic(), headers, valueByteArray);
            return new ConsumerRecord<>(partition.topic(), partition.partition(), offset,
-                                        timestamp, timestampType, record.checksumOrNull(),


Could we remove checksumOrNull from Record? That method works for AbstractLegacyRecordBatch only.

I had initially removed it. But I added it back since it's used by DumpLogSegments.

DumpLogSegments can call RecordBatch#checksum instead of checksumOrNull if the record is legacy. I can file a PR as follow-up to address that.

I don't think that suggestion works. However, you could add conditional code in DumpLogSegments so that the crc at the record level is only printed if it's AbstractLegacyRecordBatch. Sounds good as a follow-up.

Sounds good as a follow-up.

#10498

chia7712 · 2021-04-07T04:37:10Z

clients/src/test/java/org/apache/kafka/clients/producer/internals/ProducerBatchTest.java

@@ -65,7 +64,6 @@ public void testChecksumNullForMagicV2() {
        ProducerBatch batch = new ProducerBatch(new TopicPartition("topic", 1), memoryRecordsBuilder, now);
        FutureRecordMetadata future = batch.tryAppend(now, null, new byte[10], Record.EMPTY_HEADERS, null, now);
        assertNotNull(future);
-        assertNull(future.checksumOrNull());


this test case (testChecksumNullForMagicV2) is meaningless after we remove assertNull(future.checksumOrNull()). It seems to me it is ok to remove whole test case.

* apache-github/trunk: KAFKA-10769 Remove JoinGroupRequest#containsValidPattern as it is dup… (apache#9851) KAFKA-12384: stabilize ListOffsetsRequestTest#testResponseIncludesLeaderEpoch (apache#10389) KAFKA-5146: remove Connect dependency from Streams module (apache#10131)

…tency with recent change in `FutureRecordMetadata`

chia7712

LGTM

chia7712 · 2021-04-07T06:04:23Z

clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRecord.java

@@ -32,7 +31,6 @@
 public class ConsumerRecord<K, V> {
    public static final long NO_TIMESTAMP = RecordBatch.NO_TIMESTAMP;
    public static final int NULL_SIZE = -1;
-    public static final int NULL_CHECKSUM = -1;


Does this line need deprecation cycle also?

dajac · 2021-04-07T06:38:29Z

clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRecord.java

+     * @param key The key of the record, if one exists (null is allowed)
+     * @param value The record contents
+     *
+     * @deprecated use one of the constructors without a `checksum` parameter


It is a pity that we have to carry on these constructors. Should we explicitly add the deprecation version in the message here and in the others? Thinking about the future, it is a little easier to decide when they can be removed when they do.

Yeah, I agree it's annoying. Also, it would be nice to have a constructor like:

public ConsumerRecord(String topic, int partition, long offset, long timestamp, TimestampType timestampType, K key, V value)

And deprecate:

public ConsumerRecord(String topic, int partition, long offset, K key, V value) {

But that requires a KIP. For now, I will remove some of the new constructors I added and add a single one:

public ConsumerRecord(String topic, int partition, long offset, long timestamp, TimestampType timestampType, int serializedKeySize, int serializedValueSize, K key, V value, Headers headers, Optional<Integer> leaderEpoch) {

I'll update the PR with these changes tomorrow.

chia7712

+1

chia7712 · 2021-04-14T16:55:05Z

clients/src/main/java/org/apache/kafka/common/record/MemoryRecordsBuilder.java

@@ -398,7 +398,7 @@ private int writeLegacyCompressedWrapperHeader() {
    /**
     * Append a record and return its checksum for message format v0 and v1, or null for v2 and above.


Could you update this docs?

ijuma · 2021-04-14T21:37:23Z

3 jobs passed, one (Build / JDK 15 and Scala 2.13) failed with unrelated flakes:

org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplicationWithEmptyPartition()Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testOneWayReplicationWithAutoOffsetSync()Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplicationWithEmptyPartition()Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testOneWayReplicationWithAutoOffsetSync()Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testDeleteConnectorBuild / JDK 15 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic()Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testOneWayReplicationWithAutoOffsetSync()Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testOneWayReplicationWithAutoOffsetSync()Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testReplicationWithEmptyPartition()Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testReplicationWithEmptyPartition()

ijuma requested review from chia7712 and dajac April 3, 2021 18:30

dajac reviewed Apr 6, 2021

View reviewed changes

ijuma added 2 commits April 6, 2021 15:06

Add upgrade note

c059c3f

ijuma force-pushed the remove-checksum branch from 0829668 to c059c3f Compare April 6, 2021 22:08

ijuma mentioned this pull request Apr 6, 2021

KAFKA-12548; Propagate record error messages to application #10445

Merged

3 tasks

Add note about ChecksumMessageFormatter

1980652

chia7712 reviewed Apr 7, 2021

View reviewed changes

ijuma added 2 commits April 6, 2021 22:10

Address review feedback

6357b7f

chia7712 mentioned this pull request Apr 7, 2021

MINOR: remove checksumOrNull and isValid from Record #10498

Merged

3 tasks

ijuma added 2 commits April 6, 2021 22:49

Use batchIndex as the parameter name in RecordMetadata for consis…

9b22b53

…tency with recent change in `FutureRecordMetadata`

Add back ConsumerRecord constructors

bfa4f6c

ijuma force-pushed the remove-checksum branch from f3f284f to bfa4f6c Compare April 7, 2021 05:49

Remove unused imports

5192394

chia7712 approved these changes Apr 7, 2021

View reviewed changes

dajac reviewed Apr 7, 2021

View reviewed changes

ijuma added 4 commits April 14, 2021 06:26

Merge branch 'trunk' into remove-checksum

d45a4e8

ConsumerRecord improvements

4e70146

Add minimal javadoc on RecordMetadata constructors.

8585074

Improve RecordMetadataTest

69343e6

ijuma changed the title ~~KAFKA-12612: Remove checksum from ConsumerRecord and RecordMetadata for 3.0~~ KAFKA-12612: Remove checksum from ConsumerRecord/RecordMetadata for 3.0 Apr 14, 2021

chia7712 approved these changes Apr 14, 2021

View reviewed changes

ijuma added 2 commits April 14, 2021 11:43

Update javadoc

0ea7e53

Fix javadoc

2228972

ijuma merged commit 89933f2 into apache:trunk Apr 14, 2021

ijuma deleted the remove-checksum branch April 14, 2021 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-12612: Remove `checksum` from ConsumerRecord/RecordMetadata for 3.0 #10470

KAFKA-12612: Remove `checksum` from ConsumerRecord/RecordMetadata for 3.0 #10470

ijuma commented Apr 3, 2021 •

edited

Loading

ijuma commented Apr 3, 2021

ijuma commented Apr 3, 2021

dajac Apr 6, 2021

ijuma Apr 6, 2021

chia7712 left a comment

chia7712 Apr 7, 2021

ijuma Apr 7, 2021

chia7712 Apr 7, 2021

chia7712 Apr 7, 2021

ijuma Apr 7, 2021

chia7712 Apr 7, 2021

ijuma Apr 7, 2021

chia7712 Apr 7, 2021

chia7712 Apr 7, 2021

chia7712 left a comment

chia7712 Apr 7, 2021

ijuma Apr 7, 2021

dajac Apr 7, 2021

ijuma Apr 7, 2021

chia7712 left a comment

chia7712 Apr 14, 2021

ijuma commented Apr 14, 2021

		@@ -398,7 +398,7 @@ private int writeLegacyCompressedWrapperHeader() {
		/**
		* Append a record and return its checksum for message format v0 and v1, or null for v2 and above.

KAFKA-12612: Remove checksum from ConsumerRecord/RecordMetadata for 3.0 #10470

KAFKA-12612: Remove checksum from ConsumerRecord/RecordMetadata for 3.0 #10470

Conversation

ijuma commented Apr 3, 2021 • edited Loading

Committer Checklist (excluded from commit message)

ijuma commented Apr 3, 2021

ijuma commented Apr 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma commented Apr 14, 2021

KAFKA-12612: Remove `checksum` from ConsumerRecord/RecordMetadata for 3.0 #10470

KAFKA-12612: Remove `checksum` from ConsumerRecord/RecordMetadata for 3.0 #10470

ijuma commented Apr 3, 2021 •

edited

Loading