KAFKA-14240; Validate kraft snapshot state on startup by hachikuji · Pull Request #12653 · apache/kafka

hachikuji · 2022-09-16T19:55:11Z

We should prevent the metadata log from initializing in a known bad state. If the log start offset of the first segment is greater than 0, then must be a snapshot an offset greater than or equal to it order to ensure that the initialized state is complete.

Committer Checklist (excluded from commit message)

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

jsancio

Thanks for the change @hachikuji .

jsancio · 2022-09-16T20:05:49Z

+  // If the log start offset is not 0, then we must have a snapshot which covers the
+  // initial state up to the current log start offset.
+  if (log.logStartOffset > 0) {
+    val latestSnapshotId = snapshots.lastOption.map(_._1)
+    if (!latestSnapshotId.exists(snapshotId => snapshotId.offset >= log.logStartOffset)) {
+      throw new IllegalStateException("Inconsistent snapshot state: there must be a snapshot " +
+        s"at an offset larger then the current log start offset ${log.logStartOffset}, but the " +
+        s"latest snapshot is ${latestSnapshotId}")
+    }
+  }
+


What do you think about moving this check to recoverSnapshots or before calling recoverSnapshots?

recoverSnapshots makes changes to the log dir. Kafka should probably validate the log dir before making changes to it?

Makes sense.

I guess a specific case where doing this prior to recoverSnapshots might help is if we have a deleted file corresponding to a snapshot in the range we are looking for.

Yes. recoverSnapshots doesn't make a log dir invalid but it may make it "more" invalid by deleting more snapshots.

jsancio

Great tests. LGTM after one minor comment.

jsancio

LGTM if the tests are green.

showuon · 2022-09-17T02:15:23Z

2 out of 3 builds failed (terminated) without results. I've re-run it: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-12653/7/

hachikuji · 2022-09-17T13:45:12Z

Test failures look unrelated. I've triggered one more build to be on the safe side.

showuon · 2022-09-18T07:56:47Z

All 3 builds failed in the re-run build: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-12653/8/
Triggered another one: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-12653/9

hachikuji · 2022-09-18T17:21:52Z

Sigh.

[2022-09-18T10:01:25.947Z] * What went wrong:
[2022-09-18T10:01:25.947Z] Execution failed for task ':core:unitTest'.
[2022-09-18T10:01:25.947Z] > Process 'Gradle Test Executor 116' finished with non-zero exit value 1

showuon · 2022-09-19T00:03:43Z

2 out of 3 builds failed again in the latest build.
Could we consider to merge the fix in this PR: #12639 soon to solve this build issue? I've been spending time to investigate that issue and identified the root cause. IMO, merging that PR is the top priority to make our development process more efficient!

We should prevent the metadata log from initializing in a known bad state. If the log start offset of the first segment is greater than 0, then must be a snapshot an offset greater than or equal to it order to ensure that the initialized state is complete. Reviewers: José Armando García Sancio <jsancio@users.noreply.github.com>

…eptember 2022) `Jenkinsfile` was the only conflict and we ignore the changes since they are not relevant to the Confluent build. * apache-github/3.3: (61 commits) KAFKA-14214: Introduce read-write lock to StandardAuthorizer for consistent ACL reads. (apache#12628) KAFKA-14243: Temporarily disable unsafe downgrade (apache#12664) KAFKA-14240; Validate kraft snapshot state on startup (apache#12653) KAFKA-14233: disable testReloadUpdatedFilesWithoutConfigChange first to fix the build (apache#12658) KAFKA-14238; KRaft metadata log should not delete segment past the latest snapshot (apache#12655) KAFKA-14156: Built-in partitioner may create suboptimal batches (apache#12570) MINOR: Adds KRaft versions of most streams system tests (apache#12458) MINOR; Add missing li end tag (apache#12640) MINOR: Mention that kraft is production ready in upgrade notes (apache#12635) MINOR: Add upgrade note regarding the Strictly Uniform Sticky Partitioner (KIP-794) (apache#12630) KAFKA-14222; KRaft's memory pool should always allocate a buffer (apache#12625) KAFKA-14208; Do not raise wakeup in consumer during asynchronous offset commits (apache#12626) KAFKA-14196; Do not continue fetching partitions awaiting auto-commit prior to revocation (apache#12603) KAFKA-14215; Ensure forwarded requests are applied to broker request quota (apache#12624) MINOR; Remove end html tag from upgrade (apache#12605) Remove the html end tag from upgrade.html KAFKA-14205; Document how to replace the disk for the KRaft Controller (apache#12597) KAFKA-14203 Disable snapshot generation on broker after metadata errors (apache#12596) KAFKA-14216: Remove ZK reference from org.apache.kafka.server.quota.ClientQuotaCallback javadoc (apache#12617) KAFKA-14217: app-reset-tool.html should not show --zookeeper flag that no longer exists (apache#12618) ...

We should prevent the metadata log from initializing in a known bad state. If the log start offset of the first segment is greater than 0, then must be a snapshot an offset greater than or equal to it order to ensure that the initialized state is complete. Reviewers: José Armando García Sancio <jsancio@users.noreply.github.com>

KAFKA-14240; Validate kraft snapshot state on startup

e0d7316

jsancio reviewed Sep 16, 2022

View reviewed changes

hachikuji added 2 commits September 16, 2022 14:54

Move validation prior to deletion during snapshot recovery

0fe4a48

test cleanups

421b9dd

hachikuji force-pushed the KAFKA-14240 branch from 76ea1b0 to 421b9dd Compare September 16, 2022 22:01

jsancio reviewed Sep 16, 2022

View reviewed changes

Comment thread core/src/main/scala/kafka/raft/KafkaMetadataLog.scala

jsancio approved these changes Sep 16, 2022

View reviewed changes

jsancio added core Kafka Broker 3.3 labels Sep 16, 2022

Log deleted and retained snapshots during recovery

9d3b891

hachikuji force-pushed the KAFKA-14240 branch from df09abf to 9d3b891 Compare September 16, 2022 22:59

Merge remote-tracking branch 'apache/trunk' into KAFKA-14240

062cc5e

hachikuji merged commit 8c8b536 into apache:trunk Sep 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-14240; Validate kraft snapshot state on startup#12653

KAFKA-14240; Validate kraft snapshot state on startup#12653
hachikuji merged 5 commits intoapache:trunkfrom
hachikuji:KAFKA-14240

hachikuji commented Sep 16, 2022

Uh oh!

jsancio left a comment

Uh oh!

jsancio Sep 16, 2022

Uh oh!

hachikuji Sep 16, 2022

Uh oh!

hachikuji Sep 16, 2022

Uh oh!

jsancio Sep 16, 2022

Uh oh!

jsancio left a comment

Uh oh!

Uh oh!

jsancio left a comment

Uh oh!

showuon commented Sep 17, 2022

Uh oh!

hachikuji commented Sep 17, 2022

Uh oh!

showuon commented Sep 18, 2022

Uh oh!

hachikuji commented Sep 18, 2022

Uh oh!

showuon commented Sep 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

hachikuji commented Sep 16, 2022

Committer Checklist (excluded from commit message)

Uh oh!

jsancio left a comment

Choose a reason for hiding this comment

Uh oh!

jsancio Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

hachikuji Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

hachikuji Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

jsancio Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

jsancio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsancio left a comment

Choose a reason for hiding this comment

Uh oh!

showuon commented Sep 17, 2022

Uh oh!

hachikuji commented Sep 17, 2022

Uh oh!

showuon commented Sep 18, 2022

Uh oh!

hachikuji commented Sep 18, 2022

Uh oh!

showuon commented Sep 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants