You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Timothy Chen <tn...@apache.org> on 2014/03/25 19:54:08 UTC
Review Request 19626: Patch for KAFKA-1323
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1323
https://issues.apache.org/jira/browse/KAFKA-1323
Repository: kafka
Description
-------
KAFKA-1323 Fix log directory to support relative directories
Diffs
-----
core/src/main/scala/kafka/cluster/Partition.scala 0b88f14c4855b27242906cd45930bae501e26226
core/src/main/scala/kafka/server/ReplicaManager.scala 255be063ee247a849e527297c987da4625d749ca
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
Diff: https://reviews.apache.org/r/19626/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 19626: Patch for KAFKA-1323
Posted by Timothy Chen <tn...@apache.org>.
> On March 26, 2014, 12:55 a.m., Neha Narkhede wrote:
> > core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala, line 66
> > <https://reviews.apache.org/r/19626/diff/1/?file=535572#file535572line66>
> >
> > This method can be private.
> >
> > This method seems to indicate that it creates a mock log manager, but actually it creates a real LogManager object. I'm not sure I understand the motivation behind moving away from the mock object to an actual LogManager object?
Originally I just mocked LogManager with EasyMock, but encountered the problem where Partition.getOrCreateReplica is looking for properties in LogManager and seems like EasyMock only allows mocking methods not public properties.
It's not a full mock, but I'm using a mocked object (MockTime) so I named it mock log manager. Can definitely just rename the test method and make it private.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/#review38565
-----------------------------------------------------------
On March 25, 2014, 6:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19626/
> -----------------------------------------------------------
>
> (Updated March 25, 2014, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1323
> https://issues.apache.org/jira/browse/KAFKA-1323
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1323 Fix log directory to support relative directories
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 0b88f14c4855b27242906cd45930bae501e26226
> core/src/main/scala/kafka/server/ReplicaManager.scala 255be063ee247a849e527297c987da4625d749ca
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
>
> Diff: https://reviews.apache.org/r/19626/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19626: Patch for KAFKA-1323
Posted by Neha Narkhede <ne...@gmail.com>.
> On March 26, 2014, 12:55 a.m., Neha Narkhede wrote:
> > My guess is that we may have to review and make a similar change in the following places -
> > 1. LogManager.checkpointRecoveryPointOffsets()
> > 2. LogManager.nextLogDir()
> >
> > The unit test in ReplicaManagerTest cover checkpointing, but we also need to add coverage for log recovery and log cleaning since those are the places that touch the directory mapping
>
> Timothy Chen wrote:
> I just looked at the LogManager and looks like they have their own internal directory mapping that uses File objects, and LogManager constructor only accepts array of File directories so I don't think relative/absolute or different path trailing conventions has much meaning to be tested for LogMgr. Thoughts?
hmm.. checkpointRecoveryPointOffsets() also seemed to access the parent directory -
val recoveryPointsByDir = this.logsByTopicPartition.groupBy(_._2.dir.getParent.toString)
Same in nextLogDir() -
val logCounts = allLogs.groupBy(_.dir.getParent).mapValues(_.size)
val zeros = logDirs.map(dir => (dir.getPath, 0)).toMap
I'm not sure that this will necessarily be a problem, but it will be good to get unit test coverage for this bug for the above 2 APIs. In general, that would be log recovery and log cleaning.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/#review38565
-----------------------------------------------------------
On March 25, 2014, 6:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19626/
> -----------------------------------------------------------
>
> (Updated March 25, 2014, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1323
> https://issues.apache.org/jira/browse/KAFKA-1323
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1323 Fix log directory to support relative directories
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 0b88f14c4855b27242906cd45930bae501e26226
> core/src/main/scala/kafka/server/ReplicaManager.scala 255be063ee247a849e527297c987da4625d749ca
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
>
> Diff: https://reviews.apache.org/r/19626/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19626: Patch for KAFKA-1323
Posted by Timothy Chen <tn...@apache.org>.
> On March 26, 2014, 12:55 a.m., Neha Narkhede wrote:
> > My guess is that we may have to review and make a similar change in the following places -
> > 1. LogManager.checkpointRecoveryPointOffsets()
> > 2. LogManager.nextLogDir()
> >
> > The unit test in ReplicaManagerTest cover checkpointing, but we also need to add coverage for log recovery and log cleaning since those are the places that touch the directory mapping
>
> Timothy Chen wrote:
> I just looked at the LogManager and looks like they have their own internal directory mapping that uses File objects, and LogManager constructor only accepts array of File directories so I don't think relative/absolute or different path trailing conventions has much meaning to be tested for LogMgr. Thoughts?
>
> Neha Narkhede wrote:
> hmm.. checkpointRecoveryPointOffsets() also seemed to access the parent directory -
>
> val recoveryPointsByDir = this.logsByTopicPartition.groupBy(_._2.dir.getParent.toString)
>
> Same in nextLogDir() -
>
> val logCounts = allLogs.groupBy(_.dir.getParent).mapValues(_.size)
> val zeros = logDirs.map(dir => (dir.getPath, 0)).toMap
>
> I'm not sure that this will necessarily be a problem, but it will be good to get unit test coverage for this bug for the above 2 APIs. In general, that would be log recovery and log cleaning.
I think the difference really is that in ReplicaManagerTest we read in configuration log.dirs directly as string and store them in the map keys, where here they're all wrapped with File object so paths have the same convention when they are returned.
I will add Log manager tests just to verify this.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/#review38565
-----------------------------------------------------------
On March 25, 2014, 6:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19626/
> -----------------------------------------------------------
>
> (Updated March 25, 2014, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1323
> https://issues.apache.org/jira/browse/KAFKA-1323
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1323 Fix log directory to support relative directories
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 0b88f14c4855b27242906cd45930bae501e26226
> core/src/main/scala/kafka/server/ReplicaManager.scala 255be063ee247a849e527297c987da4625d749ca
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
>
> Diff: https://reviews.apache.org/r/19626/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19626: Patch for KAFKA-1323
Posted by Timothy Chen <tn...@apache.org>.
> On March 26, 2014, 12:55 a.m., Neha Narkhede wrote:
> > My guess is that we may have to review and make a similar change in the following places -
> > 1. LogManager.checkpointRecoveryPointOffsets()
> > 2. LogManager.nextLogDir()
> >
> > The unit test in ReplicaManagerTest cover checkpointing, but we also need to add coverage for log recovery and log cleaning since those are the places that touch the directory mapping
I just looked at the LogManager and looks like they have their own internal directory mapping that uses File objects, and LogManager constructor only accepts array of File directories so I don't think relative/absolute or different path trailing conventions has much meaning to be tested for LogMgr. Thoughts?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/#review38565
-----------------------------------------------------------
On March 25, 2014, 6:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19626/
> -----------------------------------------------------------
>
> (Updated March 25, 2014, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1323
> https://issues.apache.org/jira/browse/KAFKA-1323
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1323 Fix log directory to support relative directories
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 0b88f14c4855b27242906cd45930bae501e26226
> core/src/main/scala/kafka/server/ReplicaManager.scala 255be063ee247a849e527297c987da4625d749ca
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
>
> Diff: https://reviews.apache.org/r/19626/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19626: Patch for KAFKA-1323
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/#review38565
-----------------------------------------------------------
My guess is that we may have to review and make a similar change in the following places -
1. LogManager.checkpointRecoveryPointOffsets()
2. LogManager.nextLogDir()
The unit test in ReplicaManagerTest cover checkpointing, but we also need to add coverage for log recovery and log cleaning since those are the places that touch the directory mapping
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala
<https://reviews.apache.org/r/19626/#comment70833>
This method can be private.
This method seems to indicate that it creates a mock log manager, but actually it creates a real LogManager object. I'm not sure I understand the motivation behind moving away from the mock object to an actual LogManager object?
- Neha Narkhede
On March 25, 2014, 6:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19626/
> -----------------------------------------------------------
>
> (Updated March 25, 2014, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1323
> https://issues.apache.org/jira/browse/KAFKA-1323
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1323 Fix log directory to support relative directories
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 0b88f14c4855b27242906cd45930bae501e26226
> core/src/main/scala/kafka/server/ReplicaManager.scala 255be063ee247a849e527297c987da4625d749ca
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
>
> Diff: https://reviews.apache.org/r/19626/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19626: Patch for KAFKA-1323
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/#review38554
-----------------------------------------------------------
Ship it!
Ship It!
- Guozhang Wang
On March 25, 2014, 6:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19626/
> -----------------------------------------------------------
>
> (Updated March 25, 2014, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1323
> https://issues.apache.org/jira/browse/KAFKA-1323
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1323 Fix log directory to support relative directories
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 0b88f14c4855b27242906cd45930bae501e26226
> core/src/main/scala/kafka/server/ReplicaManager.scala 255be063ee247a849e527297c987da4625d749ca
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
>
> Diff: https://reviews.apache.org/r/19626/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19626: Patch for KAFKA-1323
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/#review39450
-----------------------------------------------------------
core/src/test/scala/unit/kafka/log/LogManagerTest.scala
<https://reviews.apache.org/r/19626/#comment71842>
You can use the scala intercept[] annotation for expected exception. One example is in ProducerFailureHandlingTest.
core/src/test/scala/unit/kafka/log/LogManagerTest.scala
<https://reviews.apache.org/r/19626/#comment71879>
Could we move the appending logic out of the verification function, so that we can also test the case when there is no data append to the logs, etc?
core/src/test/scala/unit/kafka/log4j/KafkaLog4jAppenderTest.scala
<https://reviews.apache.org/r/19626/#comment71880>
Why we want to change the logic to waitUntilTrue here?
- Guozhang Wang
On April 2, 2014, 7:18 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19626/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 7:18 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1323
> https://issues.apache.org/jira/browse/KAFKA-1323
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1323 Fix log directory to support relative directories
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 6d6b4ab91608162201d2737d784580728193566a
> core/src/main/scala/kafka/server/ReplicaManager.scala 0f5aaa9888d6c0ca0b073f4742bee14ad72d0484
> core/src/test/scala/unit/kafka/log/LogManagerTest.scala b4bee33191ebc0b4ab5a5f82fa232cd3bda8fef8
> core/src/test/scala/unit/kafka/log4j/KafkaLog4jAppenderTest.scala 67497dd042dfd4ad54875fe29200d67f59137264
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
>
> Diff: https://reviews.apache.org/r/19626/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19626: Patch for KAFKA-1323
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/#review39553
-----------------------------------------------------------
Ship it!
Ship It!
- Jun Rao
On April 2, 2014, 7:18 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19626/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 7:18 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1323
> https://issues.apache.org/jira/browse/KAFKA-1323
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1323 Fix log directory to support relative directories
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 6d6b4ab91608162201d2737d784580728193566a
> core/src/main/scala/kafka/server/ReplicaManager.scala 0f5aaa9888d6c0ca0b073f4742bee14ad72d0484
> core/src/test/scala/unit/kafka/log/LogManagerTest.scala b4bee33191ebc0b4ab5a5f82fa232cd3bda8fef8
> core/src/test/scala/unit/kafka/log4j/KafkaLog4jAppenderTest.scala 67497dd042dfd4ad54875fe29200d67f59137264
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
>
> Diff: https://reviews.apache.org/r/19626/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19626: Patch for KAFKA-1323
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/
-----------------------------------------------------------
(Updated April 2, 2014, 7:18 p.m.)
Review request for kafka.
Bugs: KAFKA-1323
https://issues.apache.org/jira/browse/KAFKA-1323
Repository: kafka
Description
-------
KAFKA-1323 Fix log directory to support relative directories
Diffs (updated)
-----
core/src/main/scala/kafka/cluster/Partition.scala 6d6b4ab91608162201d2737d784580728193566a
core/src/main/scala/kafka/server/ReplicaManager.scala 0f5aaa9888d6c0ca0b073f4742bee14ad72d0484
core/src/test/scala/unit/kafka/log/LogManagerTest.scala b4bee33191ebc0b4ab5a5f82fa232cd3bda8fef8
core/src/test/scala/unit/kafka/log4j/KafkaLog4jAppenderTest.scala 67497dd042dfd4ad54875fe29200d67f59137264
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
Diff: https://reviews.apache.org/r/19626/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 19626: Patch for KAFKA-1323
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/#review39158
-----------------------------------------------------------
core/src/test/scala/unit/kafka/log/LogManagerTest.scala
<https://reviews.apache.org/r/19626/#comment71524>
Instead of duplicating most of the code btw the two test cases, could we put the common code in a util function?
core/src/test/scala/unit/kafka/log/LogManagerTest.scala
<https://reviews.apache.org/r/19626/#comment71523>
Incorrect comment.
- Jun Rao
On March 31, 2014, 6:57 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19626/
> -----------------------------------------------------------
>
> (Updated March 31, 2014, 6:57 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1323
> https://issues.apache.org/jira/browse/KAFKA-1323
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1323 Fix log directory to support relative directories
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 0b88f14c4855b27242906cd45930bae501e26226
> core/src/main/scala/kafka/server/ReplicaManager.scala 255be063ee247a849e527297c987da4625d749ca
> core/src/test/scala/unit/kafka/log/LogManagerTest.scala b4bee33191ebc0b4ab5a5f82fa232cd3bda8fef8
> core/src/test/scala/unit/kafka/log4j/KafkaLog4jAppenderTest.scala 67497dd042dfd4ad54875fe29200d67f59137264
> core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
>
> Diff: https://reviews.apache.org/r/19626/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19626: Patch for KAFKA-1323
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/
-----------------------------------------------------------
(Updated March 31, 2014, 6:57 p.m.)
Review request for kafka.
Bugs: KAFKA-1323
https://issues.apache.org/jira/browse/KAFKA-1323
Repository: kafka
Description
-------
KAFKA-1323 Fix log directory to support relative directories
Diffs (updated)
-----
core/src/main/scala/kafka/cluster/Partition.scala 0b88f14c4855b27242906cd45930bae501e26226
core/src/main/scala/kafka/server/ReplicaManager.scala 255be063ee247a849e527297c987da4625d749ca
core/src/test/scala/unit/kafka/log/LogManagerTest.scala b4bee33191ebc0b4ab5a5f82fa232cd3bda8fef8
core/src/test/scala/unit/kafka/log4j/KafkaLog4jAppenderTest.scala 67497dd042dfd4ad54875fe29200d67f59137264
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
Diff: https://reviews.apache.org/r/19626/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 19626: Patch for KAFKA-1323
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19626/
-----------------------------------------------------------
(Updated March 27, 2014, 11:17 p.m.)
Review request for kafka.
Bugs: KAFKA-1323
https://issues.apache.org/jira/browse/KAFKA-1323
Repository: kafka
Description
-------
KAFKA-1323 Fix log directory to support relative directories
Diffs (updated)
-----
core/src/main/scala/kafka/cluster/Partition.scala 0b88f14c4855b27242906cd45930bae501e26226
core/src/main/scala/kafka/server/ReplicaManager.scala 255be063ee247a849e527297c987da4625d749ca
core/src/test/scala/unit/kafka/log/LogManagerTest.scala b4bee33191ebc0b4ab5a5f82fa232cd3bda8fef8
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b5936d4101b513baa805ab26361fe965bdf980aa
Diff: https://reviews.apache.org/r/19626/diff/
Testing
-------
Thanks,
Timothy Chen