MINOR: Prevent creating partition.metadata until ID can be written by jolshan · Pull Request #10041 · apache/kafka

jolshan · 2021-02-03T18:15:19Z

Currently the partition.metadata file is created when the log is created. However, clusters with older inter-broker protocols will never use this file. This PR moves the creation of the file to when we write to the file.

This PR also deletes the partition.metadata file on startup if the IBP version is lower than 2.8.

I will be looking at benchmarks to see how these changes affect LISR request processing.

Committer Checklist (excluded from commit message)

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

…les.

…MetadataFile

junrao

@jolshan : Thanks for the PR. A few comments below.

junrao

@jolshan : Thanks for the updated PR. Just one more comment below.

junrao · 2021-02-09T00:27:49Z

          logDirFailureChannel: LogDirFailureChannel,
-          private val hadCleanShutdown: Boolean = true) extends Logging with KafkaMetricsGroup {
+          private val hadCleanShutdown: Boolean = true,
+          val keepPartitionMetadataFile: Boolean = true) extends Logging with KafkaMetricsGroup {


Could we add the new param to the javadoc? In the javadoc, it would be useful to explain a bit how this helps with re-upgrade.

good point. will do!

Something like this work for an explanation?

* boolean flag to indicate whether the partition.metadata file should be kept in the * log directory. A partition.metadata file is only created when the controller's * inter-broker protocol version is at least 2.8. This file will persist the topic ID on * the broker. If inter-broker protocol is downgraded below 2.8, a topic ID may be lost * and a new ID generated upon re-upgrade. If the inter-broker protocol version is below * 2.8, partition.metadata will be deleted to avoid ID conflicts upon re-upgrade.

Yes, looks good to me.

junrao

@jolshan : Thanks for the latest PR. LGTM

Are the test failure related? Also, I guess you want this PR in the 2.8 branch too?

jolshan · 2021-02-09T22:15:13Z

Yes. I will want this on the 2.8 branch. I'll check the failed tests. I've also had trouble with building at least one of the three JDKs, but it seems like it is not the same one each time.

jolshan · 2021-02-09T22:37:05Z

MirrorConnectorsIntegrationSSLTest.testOneWayReplicationWithAutoOffsetSync() seems to be flaky (is failing on other open PRs) It passed when I ran locally.
ConnectionQuotasTest.testListenerConnectionRateLimitWhenActualRateAboveLimit() seems to be unrelated. I also ran locally and it passed.

junrao · 2021-02-09T23:04:11Z

@jolshan : Thanks. For those transient failures, could you file a jira if it's not tracked already?

Are the JDK 8 failures also transient?

jolshan · 2021-02-09T23:25:21Z

I had 2 JDK 11 failures previously. I also noticed JDK failures on other PRs. I'll check JIRA for these issues

jolshan · 2021-02-09T23:45:52Z

Looks like one was already created.
https://issues.apache.org/jira/browse/KAFKA-12284
https://issues.apache.org/jira/browse/KAFKA-12319

junrao · 2021-02-10T00:02:18Z

@jolshan : Thanks. Were the JDK 8 tests ok too?

jolshan · 2021-02-10T00:36:17Z

@junrao On the previous commit JDK8 built and only MirrorConnectorsIntegrationSSLTest.testOneWayReplicationWithAutoOffsetSync() failed. (The only difference between this commit and the most recent was the javadoc change. Before that, all JDK8 tests passed.

junrao · 2021-02-10T17:16:42Z

@jolshan Could you rebase the PR for trunk? Also, I am not sure if the PR ports to 2.8 cleanly, if not, could you submit a separate PR for 2.8? Thanks.

…MetadataFile

junrao · 2021-02-10T21:41:02Z

@jolshan : Do you have any benchmark results that you want to share?

jolshan · 2021-02-10T22:09:22Z

Sure. Using the LeaderAndIsrBenchmark (and the async profiler) LISRbench.zip
from #10071 I had these results:

Benchmark                                                          (partitionCount)  (topicCount)  Mode  Cnt        Score       Error  Units
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       10            10  avgt   15    50920.020 ±  6450.121  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       10            20  avgt   15    81505.310 ±  3821.451  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       10           100  avgt   15   451907.331 ±  2157.775  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       20            10  avgt   15    75708.157 ±  4316.357  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       20            20  avgt   15   136747.799 ±  3886.797  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       20           100  avgt   15   980846.811 ±  4017.757  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       50            10  avgt   15   156896.027 ±  4952.014  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       50            20  avgt   15   318294.323 ±  4729.402  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       50           100  avgt   15  2904827.652 ± 74264.592  ns/op

I took a look at the flame graph for these tests and the file I/O impact seemed to be very minimal

This was trunk before the changes:

Benchmark                                                          (partitionCount)  (topicCount)  Mode  Cnt        Score        Error  Units
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       10            10  avgt   15    53887.240 ±   5645.067  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       10            20  avgt   15    83015.916 ±   3560.633  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       10           100  avgt   15   481109.947 ±  18767.713  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       20            10  avgt   15    75932.544 ±   6528.281  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       20            20  avgt   15   148323.073 ±   6009.839  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       20           100  avgt   15  1146604.372 ±  54073.673  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       50            10  avgt   15   171529.923 ±   9940.990  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       50            20  avgt   15   327926.279 ±   9292.803  ns/op
LeaderAndIsrRequestBenchmark.testHandleLeaderAndIsrRequest                       50           100  avgt   15  3512983.407 ± 570652.231  ns/op

junrao · 2021-02-10T23:26:10Z

@jolshan : It seems this PR can't be applied cleanly in 2.8. Could you submit a separate PR for 2.8? Thanks.

jolshan · 2021-02-10T23:49:26Z

@junrao #10100

…e-allocations-lz4 * apache-github/trunk: (118 commits) KAFKA-12327: Remove MethodHandle usage in CompressionType (apache#10123) KAFKA-12297: Make MockProducer return RecordMetadata with values as per contract MINOR: Update zstd and use classes with no finalizers (apache#10120) KAFKA-12326: Corrected regresion in MirrorMaker 2 executable introduced with KAFKA-10021 (apache#10122) KAFKA-12321 the comparison function for uuid type should be 'equals' rather than '==' (apache#10098) MINOR: Add FetchSnapshot API doc in KafkaRaftClient (apache#10097) MINOR: KIP-631 KafkaConfig fixes and improvements (apache#10114) KAFKA-12272: Fix commit-interval metrics (apache#10102) MINOR: Improve confusing admin client shutdown logging (apache#10107) MINOR: Add BrokerMetadataListener (apache#10111) MINOR: Support Raft-based metadata quorums in system tests (apache#10093) MINOR: add the MetaLogListener, LocalLogManager, and Controller interface. (apache#10106) MINOR: Introduce the KIP-500 Broker lifecycle manager (apache#10095) MINOR: Remove always-passing validation in TestRecordTest#testProducerRecord (apache#9930) KAFKA-5235: GetOffsetShell: Support for multiple topics and consumer configuration override (KIP-635) (apache#9430) MINOR: Prevent creating partition.metadata until ID can be written (apache#10041) MINOR: Add RaftReplicaManager (apache#10069) MINOR: Add ClientQuotaMetadataManager for processing QuotaRecord (apache#10101) MINOR: Rename DecommissionBrokers to UnregisterBrokers (apache#10084) MINOR: KafkaBroker.brokerState should be volatile instead of AtomicReference (apache#10080) ... clients/src/main/java/org/apache/kafka/common/record/CompressionType.java core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala

jolshan added 3 commits February 3, 2021 10:03

Moved creation of file to write to prevent unnecessary creation of fi…

67f2701

…les.

Now will delete partition.metadata if IBP is too low.

6d8e314

Minor tweak for efficiency

58ee278

jolshan force-pushed the cleanupPartitionMetadataFile branch from 603a074 to 58ee278 Compare February 5, 2021 16:49

Merge branch 'trunk' of github.com:apache/kafka into cleanupPartition…

a0cb9b5

…MetadataFile

junrao reviewed Feb 8, 2021

View reviewed changes

Comment thread core/src/main/scala/kafka/log/Log.scala Outdated

Comment thread core/src/main/scala/kafka/log/Log.scala Outdated

Comment thread core/src/main/scala/kafka/server/PartitionMetadataFile.scala Outdated

Comment thread core/src/test/scala/unit/kafka/log/LogManagerTest.scala Outdated

Addressing comments.

18d5a8e

junrao reviewed Feb 9, 2021

View reviewed changes

Added comment to explain keepPartitionMetadataFile flag

a36521d

junrao approved these changes Feb 9, 2021

View reviewed changes

Merge branch 'trunk' of github.com:apache/kafka into cleanupPartition…

1da8c72

…MetadataFile

jolshan mentioned this pull request Feb 10, 2021

MINOR: Prevent creating partition.metadata until ID can be written #10100

Merged

3 tasks

junrao merged commit 39dcdef into apache:trunk Feb 10, 2021

Conversation

jolshan commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

junrao Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

jolshan Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

jolshan Feb 9, 2021 • 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!

junrao Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

jolshan commented Feb 9, 2021

Uh oh!

jolshan commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junrao commented Feb 9, 2021

Uh oh!

jolshan commented Feb 9, 2021

Uh oh!

jolshan commented Feb 9, 2021

Uh oh!

junrao commented Feb 10, 2021

Uh oh!

jolshan commented Feb 10, 2021

Uh oh!

junrao commented Feb 10, 2021

Uh oh!

junrao commented Feb 10, 2021

Uh oh!

jolshan commented Feb 10, 2021

Uh oh!

junrao commented Feb 10, 2021

Uh oh!

jolshan commented Feb 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jolshan commented Feb 3, 2021 •

edited

Loading

jolshan Feb 9, 2021 •

edited

Loading

jolshan commented Feb 9, 2021 •

edited

Loading