diff --git a/clients/src/main/java/org/apache/kafka/common/record/MemoryRecordsBuilder.java b/clients/src/main/java/org/apache/kafka/common/record/MemoryRecordsBuilder.java index 054fb86199884..fd5a680cda835 100644 --- a/clients/src/main/java/org/apache/kafka/common/record/MemoryRecordsBuilder.java +++ b/clients/src/main/java/org/apache/kafka/common/record/MemoryRecordsBuilder.java @@ -20,6 +20,7 @@ import org.apache.kafka.common.header.Header; import org.apache.kafka.common.protocol.types.Struct; import org.apache.kafka.common.utils.ByteBufferOutputStream; +import org.apache.kafka.common.utils.Utils; import java.io.DataOutputStream; import java.io.IOException; @@ -587,6 +588,35 @@ public void appendUncheckedWithOffset(long offset, LegacyRecord record) { } } + /** + * Append a record without doing offset/magic validation (this should only be used in testing). + * + * @param offset The offset of the record + * @param record The record to add + */ + public void appendUncheckedWithOffset(long offset, SimpleRecord record) throws IOException { + if (magic >= RecordBatch.MAGIC_VALUE_V2) { + int offsetDelta = (int) (offset - baseOffset); + long timestamp = record.timestamp(); + if (firstTimestamp == null) + firstTimestamp = timestamp; + + int sizeInBytes = DefaultRecord.writeTo(appendStream, + offsetDelta, + timestamp - firstTimestamp, + record.key(), + record.value(), + record.headers()); + recordWritten(offset, timestamp, sizeInBytes); + } else { + LegacyRecord legacyRecord = LegacyRecord.create(magic, + record.timestamp(), + Utils.toNullableArray(record.key()), + Utils.toNullableArray(record.value())); + appendUncheckedWithOffset(offset, legacyRecord); + } + } + /** * Append a record at the next sequential offset. * @param record the record to add diff --git a/core/src/main/scala/kafka/log/LogValidator.scala b/core/src/main/scala/kafka/log/LogValidator.scala index 4a3077709d528..8e1744683588f 100644 --- a/core/src/main/scala/kafka/log/LogValidator.scala +++ b/core/src/main/scala/kafka/log/LogValidator.scala @@ -366,16 +366,6 @@ private[log] object LogValidator extends Logging { else None } - def validateOffset(batchIndex: Int, record: Record, expectedOffset: Long): Option[ApiRecordError] = { - // inner records offset should always be continuous - if (record.offset != expectedOffset) { - brokerTopicStats.allTopicsStats.invalidOffsetOrSequenceRecordsPerSec.mark() - Some(ApiRecordError(Errors.INVALID_RECORD, new RecordError(batchIndex, - s"Inner record $record inside the compressed record batch does not have " + - s"incremental offsets, expected offset is $expectedOffset in topic partition $topicPartition."))) - } else None - } - // No in place assignment situation 1 var inPlaceAssignment = sourceCodec == targetCodec @@ -423,8 +413,15 @@ private[log] object LogValidator extends Logging { if (batch.magic > RecordBatch.MAGIC_VALUE_V0 && toMagic > RecordBatch.MAGIC_VALUE_V0) { if (record.timestamp > maxTimestamp) maxTimestamp = record.timestamp - validateOffset(batchIndex, record, expectedOffset) - } else None + + // Some older clients do not implement the V1 internal offsets correctly. + // Historically the broker handled this by rewriting the batches rather + // than rejecting the request. We must continue this handling here to avoid + // breaking these clients. + if (record.offset != expectedOffset) + inPlaceAssignment = false + } + None } } diff --git a/core/src/test/scala/unit/kafka/log/LogValidatorTest.scala b/core/src/test/scala/unit/kafka/log/LogValidatorTest.scala index fe258c01a8286..f63d902810025 100644 --- a/core/src/test/scala/unit/kafka/log/LogValidatorTest.scala +++ b/core/src/test/scala/unit/kafka/log/LogValidatorTest.scala @@ -21,15 +21,15 @@ import java.util.concurrent.TimeUnit import kafka.api.{ApiVersion, KAFKA_2_0_IV1, KAFKA_2_3_IV1} import kafka.common.{LongRef, RecordValidationException} +import kafka.log.LogValidator.ValidationAndOffsetAssignResult import kafka.message._ import kafka.metrics.KafkaYammerMetrics import kafka.server.BrokerTopicStats import kafka.utils.TestUtils.meterCount -import org.apache.kafka.common.InvalidRecordException -import org.apache.kafka.common.TopicPartition import org.apache.kafka.common.errors.{InvalidTimestampException, UnsupportedCompressionTypeException, UnsupportedForMessageFormatException} import org.apache.kafka.common.record._ import org.apache.kafka.common.utils.Time +import org.apache.kafka.common.{InvalidRecordException, TopicPartition} import org.apache.kafka.test.TestUtils import org.junit.Assert._ import org.junit.Test @@ -64,6 +64,29 @@ class LogValidatorTest { checkAllowMultiBatch(RecordBatch.MAGIC_VALUE_V1, CompressionType.NONE, CompressionType.GZIP) } + @Test + def testValidationOfBatchesWithNonSequentialInnerOffsets(): Unit = { + def testMessageValidation(magicValue: Byte): Unit = { + val numRecords = 20 + val invalidRecords = recordsWithNonSequentialInnerOffsets(magicValue, CompressionType.GZIP, numRecords) + + // Validation for v2 and above is strict for this case. For older formats, we fix invalid + // internal offsets by rewriting the batch. + if (magicValue >= RecordBatch.MAGIC_VALUE_V2) { + assertThrows[InvalidRecordException] { + validateMessages(invalidRecords, magicValue, CompressionType.GZIP, CompressionType.GZIP) + } + } else { + val result = validateMessages(invalidRecords, magicValue, CompressionType.GZIP, CompressionType.GZIP) + assertEquals(0 until numRecords, result.validatedRecords.records.asScala.map(_.offset)) + } + } + + for (version <- RecordVersion.values) { + testMessageValidation(version.value) + } + } + @Test def testMisMatchMagic(): Unit = { checkMismatchMagic(RecordBatch.MAGIC_VALUE_V0, RecordBatch.MAGIC_VALUE_V1, CompressionType.GZIP) @@ -88,7 +111,10 @@ class LogValidatorTest { assertTrue(meterCount(s"${BrokerTopicStats.InvalidMagicNumberRecordsPerSec}") > 0) } - private def validateMessages(records: MemoryRecords, magic: Byte, sourceCompressionType: CompressionType, targetCompressionType: CompressionType): Unit = { + private def validateMessages(records: MemoryRecords, + magic: Byte, + sourceCompressionType: CompressionType, + targetCompressionType: CompressionType): ValidationAndOffsetAssignResult = { LogValidator.validateMessagesAndAssignOffsets(records, topicPartition, new LongRef(0L), @@ -1404,6 +1430,23 @@ class LogValidatorTest { } } + private def recordsWithNonSequentialInnerOffsets(magicValue: Byte, + codec: CompressionType, + numRecords: Int): MemoryRecords = { + val records = (0 until numRecords).map { id => + new SimpleRecord(id.toString.getBytes) + } + + val buffer = ByteBuffer.allocate(1024) + val builder = MemoryRecords.builder(buffer, magicValue, codec, TimestampType.CREATE_TIME, 0L) + + records.foreach { record => + builder.appendUncheckedWithOffset(0, record) + } + + builder.build() + } + private def recordsWithInvalidInnerMagic(batchMagicValue: Byte, recordMagicValue: Byte, codec: CompressionType): MemoryRecords = {