KAFKA-9669; Loosen validation of inner offsets for older message formats by hachikuji · Pull Request #8647 · apache/kafka

hachikuji · 2020-05-12T00:49:57Z

Prior to KAFKA-8106, we allowed the v0 and v1 message formats to contain non-consecutive inner offsets. Inside LogValidator, we would detect this case and rewrite the batch. After KAFKA-8106, we changed the logic to raise an error in the case of the v1 message format (v0 was still expected to be rewritten). This caused an incompatibility for older clients which were depending on the looser validation. This patch reverts the old logic of rewriting the batch to fix the invalid inner offsets.

Note that the v2 message format has always had stricter validation. This patch also adds a test case for this.

Committer Checklist (excluded from commit message)

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

…at versions

jsancio

Thanks for the PR. Looks good based on my understanding. Just have a few questions.

jsancio · 2020-05-12T03:42:21Z

@@ -423,8 +413,11 @@ private[log] object LogValidator extends Logging {
              if (batch.magic > RecordBatch.MAGIC_VALUE_V0 && toMagic > RecordBatch.MAGIC_VALUE_V0) {


First, I don't fully understand the different between version 0, 1, and 2. I am concern if this check (if statement) it still accurate.

Why do we only update the maxTimestamp if the version is not v0?

Why do we disallow in place assignment of records with invalid offsets if the version is not v0?

For 1, is it because we are only allow to update it in v1 and v2? E.g.

if (toMagic >= RecordBatch.MAGIC_VALUE_V1) batch.setMaxTimestamp(timestampType, maxTimestamp)

For 2. is it because in place assignment is always false for v0? E.g.:

if (firstBatch.magic != toMagic || toMagic == RecordBatch.MAGIC_VALUE_V0) inPlaceAssignment = false

For 1), it is because the v0 format has no field for the timestamp. For 2), you are right: we never do in place assignment for v0 because it does not support relative internal offsets.

ijuma · 2020-05-12T16:02:03Z

+     * @param offset The offset of the record
+     * @param record The record to add
+     */
+    public void appendUncheckedWithOffset(long offset, SimpleRecord record) throws IOException {


I'm a bit nervous about this method being in the public interface. Could we make it package private and then add a test utility method in the same package that calls it, exposing it only for tests?

Oh, we already have a public appendUncheckedWithOffset for LegacyRecord.

We could move these methods to a TestMemoryRecordsBuilder or something. I think risk of misuse is probably not high though.

Makes sense. Since we already had one such method, I'm ok with keeping as is.

ijuma

Thanks for the PR, LGTM. Just a couple of nits below.

ijuma · 2020-05-12T16:29:07Z

-                validateOffset(batchIndex, record, expectedOffset)
-              } else None
+
+                if (record.offset != expectedOffset)


Can we add a comment here?

ijuma · 2020-05-12T16:30:02Z

+        }
+      } else {
+        val result = validateMessages(invalidRecords, magicValue, CompressionType.GZIP, CompressionType.GZIP)
+        assertEquals(0 until 20, result.validatedRecords.records.asScala.map(_.offset))


Nit: Replace 20 with numRecords

…ats (#8647) Prior to KAFKA-8106, we allowed the v0 and v1 message formats to contain non-consecutive inner offsets. Inside `LogValidator`, we would detect this case and rewrite the batch. After KAFKA-8106, we changed the logic to raise an error in the case of the v1 message format (v0 was still expected to be rewritten). This caused an incompatibility for older clients which were depending on the looser validation. This patch reverts the old logic of rewriting the batch to fix the invalid inner offsets. Note that the v2 message format has always had stricter validation. This patch also adds a test case for this. Reviewers: José Armando García Sancio <jsancio@users.noreply.github.com>, Ismael Juma <ismael@juma.me.uk>

* 'trunk' of github.com:apache/kafka: MINOR: add option to rebuild source for system tests (apache#6656) KAFKA-9850 Move KStream#repartition operator validation during Topolo… (apache#8550) MINOR: Add a duplicate() method to Message classes (apache#8556) KAFKA-9966: add internal assignment listener to stabilize eos-beta upgrade test (apache#8648) MINOR: Replace null with an actual value for timestamp field in InsertField SMT unit tests (apache#8649) MINOR: Fix ProcessorContext JavaDocs and stream-time computation (apache#8603) MINOR: improve tests for TopologyTestDriver (apache#8631) KAFKA-9821: consolidate Streams rebalance triggering mechanisms (apache#8596) KAFKA-9669; Loosen validation of inner offsets for older message formats (apache#8647) KAFKA-8770: KIP-557: Drop idempotent KTable source updates (apache#8254) MINOR: Remove allow concurrent test (apache#8641)

…ats (apache#8647) Prior to KAFKA-8106, we allowed the v0 and v1 message formats to contain non-consecutive inner offsets. Inside `LogValidator`, we would detect this case and rewrite the batch. After KAFKA-8106, we changed the logic to raise an error in the case of the v1 message format (v0 was still expected to be rewritten). This caused an incompatibility for older clients which were depending on the looser validation. This patch reverts the old logic of rewriting the batch to fix the invalid inner offsets. Note that the v2 message format has always had stricter validation. This patch also adds a test case for this. Reviewers: José Armando García Sancio <jsancio@users.noreply.github.com>, Ismael Juma <ismael@juma.me.uk>

KAFKA-9669; Loosen validation of inner offsets for older message form…

3bfde0f

…at versions

hachikuji force-pushed the KAFKA-9669 branch from bc3fcb1 to 3bfde0f Compare May 12, 2020 00:58

jsancio reviewed May 12, 2020

View reviewed changes

ijuma reviewed May 12, 2020

View reviewed changes

ijuma approved these changes May 12, 2020

View reviewed changes

Add comment about internal offset validation

b174f7c

hachikuji merged commit 81cf3fa into apache:trunk May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

KAFKA-9669; Loosen validation of inner offsets for older message formats#8647

KAFKA-9669; Loosen validation of inner offsets for older message formats#8647
hachikuji merged 2 commits into
apache:trunkfrom
hachikuji:KAFKA-9669

hachikuji commented May 12, 2020 •

edited

Loading

Uh oh!

jsancio left a comment

Uh oh!

jsancio May 12, 2020

Uh oh!

hachikuji May 12, 2020 •

edited

Loading

Uh oh!

ijuma May 12, 2020

Uh oh!

ijuma May 12, 2020

Uh oh!

hachikuji May 12, 2020 •

edited

Loading

Uh oh!

ijuma May 12, 2020

Uh oh!

ijuma left a comment

Uh oh!

ijuma May 12, 2020

Uh oh!

ijuma May 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -423,8 +413,11 @@ private[log] object LogValidator extends Logging {
		if (batch.magic > RecordBatch.MAGIC_VALUE_V0 && toMagic > RecordBatch.MAGIC_VALUE_V0) {

Uh oh!

Conversation

hachikuji commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

jsancio left a comment

Choose a reason for hiding this comment

Uh oh!

jsancio May 12, 2020

Choose a reason for hiding this comment

Uh oh!

hachikuji May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma May 12, 2020

Choose a reason for hiding this comment

Uh oh!

ijuma May 12, 2020

Choose a reason for hiding this comment

Uh oh!

hachikuji May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma May 12, 2020

Choose a reason for hiding this comment

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma May 12, 2020

Choose a reason for hiding this comment

Uh oh!

ijuma May 12, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hachikuji commented May 12, 2020 •

edited

Loading

hachikuji May 12, 2020 •

edited

Loading

hachikuji May 12, 2020 •

edited

Loading