You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/03 18:15:27 UTC
[GitHub] [kafka] jolshan opened a new pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
jolshan opened a new pull request #10041:
URL: https://github.com/apache/kafka/pull/10041
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.
I considered also gating the creation of the file on the IBP of the broker receiving the LeaderAndIsrRequest, rather than just checking if the request has a valid ID. There are two cases where the IBP of the broker could be lower than the controller that sent the request. Both cases are rare since they likely require creation of a topic during an upgrade/downgrade and specific brokers to have different IBPs.
1. The upgrade case: in this case, the broker will eventually become the higher IBP and thus gets the file slightly earlier.
2. The downgrade case: in this case, the topic ID has been written to ZK. If the topic changes, before re-upgrading the user will need to delete the file.
If it seems better to check the IBP of the broker before writing a file, let me know.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-776280666
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572450889
##########
File path: core/src/test/scala/unit/kafka/log/LogManagerTest.scala
##########
@@ -217,6 +217,7 @@ class LogManagerTest {
}
assertTrue(log.numberOfSegments > 1, "There should be more than one segment now.")
log.updateHighWatermark(log.logEndOffset)
+ log.partitionMetadataFile.get.write(Uuid.randomUuid())
Review comment:
That's what I am thinking. It's kind of weird to add the new file just to get the existing math work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572419692
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -256,7 +256,8 @@ class Log(@volatile private var _dir: File,
val topicPartition: TopicPartition,
val producerStateManager: ProducerStateManager,
logDirFailureChannel: LogDirFailureChannel,
- private val hadCleanShutdown: Boolean = true) extends Logging with KafkaMetricsGroup {
+ private val hadCleanShutdown: Boolean = true,
+ val usesTopicId: Boolean = true) extends Logging with KafkaMetricsGroup {
Review comment:
I named this based on the config's name. But the name you suggested is more descriptive.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
junrao commented on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-777104678
@jolshan : It seems this PR can't be applied cleanly in 2.8. Could you submit a separate PR for 2.8? Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572482465
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -256,7 +256,8 @@ class Log(@volatile private var _dir: File,
val topicPartition: TopicPartition,
val producerStateManager: ProducerStateManager,
logDirFailureChannel: LogDirFailureChannel,
- private val hadCleanShutdown: Boolean = true) extends Logging with KafkaMetricsGroup {
+ private val hadCleanShutdown: Boolean = true,
+ val keepPartitionMetadataFile: Boolean = true) extends Logging with KafkaMetricsGroup {
Review comment:
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.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572510600
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -256,7 +256,8 @@ class Log(@volatile private var _dir: File,
val topicPartition: TopicPartition,
val producerStateManager: ProducerStateManager,
logDirFailureChannel: LogDirFailureChannel,
- private val hadCleanShutdown: Boolean = true) extends Logging with KafkaMetricsGroup {
+ private val hadCleanShutdown: Boolean = true,
+ val keepPartitionMetadataFile: Boolean = true) extends Logging with KafkaMetricsGroup {
Review comment:
Yes, looks good to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572445245
##########
File path: core/src/test/scala/unit/kafka/log/LogManagerTest.scala
##########
@@ -217,6 +217,7 @@ class LogManagerTest {
}
assertTrue(log.numberOfSegments > 1, "There should be more than one segment now.")
log.updateHighWatermark(log.logEndOffset)
+ log.partitionMetadataFile.get.write(Uuid.randomUuid())
Review comment:
So remove the creation and subtract one file?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan edited a comment on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan edited a comment on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-776291259
`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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572450889
##########
File path: core/src/test/scala/unit/kafka/log/LogManagerTest.scala
##########
@@ -217,6 +217,7 @@ class LogManagerTest {
}
assertTrue(log.numberOfSegments > 1, "There should be more than one segment now.")
log.updateHighWatermark(log.logEndOffset)
+ log.partitionMetadataFile.get.write(Uuid.randomUuid())
Review comment:
That's what I am thinking. It's kind of weird to add the new file just to get the existing math works.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572420716
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -341,10 +342,15 @@ class Log(@volatile private var _dir: File,
producerStateManager.removeStraySnapshots(segments.values().asScala.map(_.baseOffset).toSeq)
loadProducerState(logEndOffset, reloadFromCleanShutdown = hadCleanShutdown)
- // Recover topic ID if present
+ // Delete partition metadata file if the version does not support topic IDs.
+ // Recover topic ID if present and topic IDs are supported
partitionMetadataFile.foreach { file =>
- if (!file.isEmpty())
- topicId = file.read().topicId
+ if (file.exists()) {
+ if (!usesTopicId)
+ file.delete()
Review comment:
I modeled this off of `leaderEpochCache` which is also an option. But I see how that is different now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
junrao commented on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-776303090
@jolshan : Thanks. For those transient failures, could you file a jira if it's not tracked already?
Are the JDK 8 failures also transient?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
junrao commented on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-776872795
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-776344485
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-776313326
I had 2 JDK 11 failures previously. I also noticed JDK failures on other PRs. I'll check JIRA for these issues
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao merged pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
junrao merged pull request #10041:
URL: https://github.com/apache/kafka/pull/10041
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-777118068
@junrao https://github.com/apache/kafka/pull/10100
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572474952
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -256,7 +256,8 @@ class Log(@volatile private var _dir: File,
val topicPartition: TopicPartition,
val producerStateManager: ProducerStateManager,
logDirFailureChannel: LogDirFailureChannel,
- private val hadCleanShutdown: Boolean = true) extends Logging with KafkaMetricsGroup {
+ private val hadCleanShutdown: Boolean = true,
+ val keepPartitionMetadataFile: Boolean = true) extends Logging with KafkaMetricsGroup {
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-776322170
Looks like one was already created.
https://issues.apache.org/jira/browse/KAFKA-12284
https://issues.apache.org/jira/browse/KAFKA-12319
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572444405
##########
File path: core/src/test/scala/unit/kafka/log/LogManagerTest.scala
##########
@@ -217,6 +217,7 @@ class LogManagerTest {
}
assertTrue(log.numberOfSegments > 1, "There should be more than one segment now.")
log.updateHighWatermark(log.logEndOffset)
+ log.partitionMetadataFile.get.write(Uuid.randomUuid())
Review comment:
Got it. Since the partitionMetadataFile is now created on demand, perhaps we could just change the math on the expected number of files?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
junrao commented on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-777054798
@jolshan : Do you have any benchmark results that you want to share?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572421631
##########
File path: core/src/test/scala/unit/kafka/log/LogManagerTest.scala
##########
@@ -217,6 +217,7 @@ class LogManagerTest {
}
assertTrue(log.numberOfSegments > 1, "There should be more than one segment now.")
log.updateHighWatermark(log.logEndOffset)
+ log.partitionMetadataFile.get.write(Uuid.randomUuid())
Review comment:
I wanted to test that the file is actually getting deleted. In order to check it gets deleted it has to be created.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572421631
##########
File path: core/src/test/scala/unit/kafka/log/LogManagerTest.scala
##########
@@ -217,6 +217,7 @@ class LogManagerTest {
}
assertTrue(log.numberOfSegments > 1, "There should be more than one segment now.")
log.updateHighWatermark(log.logEndOffset)
+ log.partitionMetadataFile.get.write(Uuid.randomUuid())
Review comment:
I wanted to test the correct files are kept/deleted. I could also change the math in line 234 if we don't need this test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572399914
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -341,10 +342,15 @@ class Log(@volatile private var _dir: File,
producerStateManager.removeStraySnapshots(segments.values().asScala.map(_.baseOffset).toSeq)
loadProducerState(logEndOffset, reloadFromCleanShutdown = hadCleanShutdown)
- // Recover topic ID if present
+ // Delete partition metadata file if the version does not support topic IDs.
+ // Recover topic ID if present and topic IDs are supported
partitionMetadataFile.foreach { file =>
- if (!file.isEmpty())
- topicId = file.read().topicId
+ if (file.exists()) {
+ if (!usesTopicId)
+ file.delete()
Review comment:
Does partitionMetadataFile need to be of Some? It seems that we can just always instantiate the object.
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -256,7 +256,8 @@ class Log(@volatile private var _dir: File,
val topicPartition: TopicPartition,
val producerStateManager: ProducerStateManager,
logDirFailureChannel: LogDirFailureChannel,
- private val hadCleanShutdown: Boolean = true) extends Logging with KafkaMetricsGroup {
+ private val hadCleanShutdown: Boolean = true,
+ val usesTopicId: Boolean = true) extends Logging with KafkaMetricsGroup {
Review comment:
Could we add the new param to javadoc? Also, will keepPartitionMetdataFile be better than usesTopicId?
##########
File path: core/src/main/scala/kafka/server/PartitionMetadataFile.scala
##########
@@ -91,11 +91,10 @@ class PartitionMetadataFile(val file: File,
private val lock = new Object()
private val logDir = file.getParentFile.getParent
-
- try Files.createFile(file.toPath) // create the file if it doesn't exist
- catch { case _: FileAlreadyExistsException => }
-
def write(topicId: Uuid): Unit = {
+ try Files.createFile(file.toPath) // create the file if it doesn't exist
Review comment:
Do we need to create the file first? It seems that later on we always rename the temp file to this one.
##########
File path: core/src/test/scala/unit/kafka/log/LogManagerTest.scala
##########
@@ -217,6 +217,7 @@ class LogManagerTest {
}
assertTrue(log.numberOfSegments > 1, "There should be more than one segment now.")
log.updateHighWatermark(log.logEndOffset)
+ log.partitionMetadataFile.get.write(Uuid.randomUuid())
Review comment:
Is this needed? It seems Log never reads UUID? Ditto below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572482465
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -256,7 +256,8 @@ class Log(@volatile private var _dir: File,
val topicPartition: TopicPartition,
val producerStateManager: ProducerStateManager,
logDirFailureChannel: LogDirFailureChannel,
- private val hadCleanShutdown: Boolean = true) extends Logging with KafkaMetricsGroup {
+ private val hadCleanShutdown: Boolean = true,
+ val keepPartitionMetadataFile: Boolean = true) extends Logging with KafkaMetricsGroup {
Review comment:
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.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-777069233
Sure. Using the LeaderAndIsrBenchmark (and the async profiler) [LISRbench.zip](https://github.com/apache/kafka/files/5961818/LISRbench.zip)
from https://github.com/apache/kafka/pull/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
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-776291259
`MirrorConnectorsIntegrationSSLTest.testOneWayReplicationWithAutoOffsetSync()` seems to be flaky (is failing on other open PRs)
`ConnectionQuotasTest.testListenerConnectionRateLimitWhenActualRateAboveLimit()` seems to be unrelated. I also ran locally and it passed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
junrao commented on pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#issuecomment-776328567
@jolshan : Thanks. Were the JDK 8 tests ok too?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a change in pull request #10041: MINOR: Prevent creating partition.metadata until ID can be written
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10041:
URL: https://github.com/apache/kafka/pull/10041#discussion_r572475420
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -256,7 +256,8 @@ class Log(@volatile private var _dir: File,
val topicPartition: TopicPartition,
val producerStateManager: ProducerStateManager,
logDirFailureChannel: LogDirFailureChannel,
- private val hadCleanShutdown: Boolean = true) extends Logging with KafkaMetricsGroup {
+ private val hadCleanShutdown: Boolean = true,
+ val keepPartitionMetadataFile: Boolean = true) extends Logging with KafkaMetricsGroup {
Review comment:
good point. will do!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org