You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Gwen Shapira <gs...@cloudera.com> on 2014/12/18 19:59:16 UTC

Review Request 29210: Patch for KAFKA-1819

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/
-----------------------------------------------------------

Review request for kafka.


Bugs: KAFKA-1819
    https://issues.apache.org/jira/browse/KAFKA-1819


Repository: kafka


Description
-------

added locking


Diffs
-----

  core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
  core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
  core/src/main/scala/kafka/log/LogManager.scala 4d2924d04bc4bd62413edb0ee2d4aaf3c0052867 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 

Diff: https://reviews.apache.org/r/29210/diff/


Testing
-------


Thanks,

Gwen Shapira

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Gwen Shapira <gs...@cloudera.com>.

> On Dec. 29, 2014, 10:14 p.m., Neha Narkhede wrote:
> > core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala, line 75
> > <https://reviews.apache.org/r/29210/diff/2/?file=801625#file801625line75>
> >
> >     Gwen, I suggest adding this check (reading the checkpoint file and ensuring that the entries related to the deleted topic are gone) to the DeleteTopicTest. 
> >     
> >     Rest of the patch looks good. We can check it in after this change

Thanks. Added a cleaner test to DeleteTopicTest.


- Gwen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/#review66307
-----------------------------------------------------------


On Dec. 31, 2014, 12:01 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29210/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2014, 12:01 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1819
>     https://issues.apache.org/jira/browse/KAFKA-1819
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> added locking
> 
> 
> improved tests per Joel and Neha's suggestions
> 
> 
> added cleaner test to DeleteTopicTest
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
>   core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
>   core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 
> 
> Diff: https://reviews.apache.org/r/29210/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/#review66307
-----------------------------------------------------------



core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala
<https://reviews.apache.org/r/29210/#comment109701>

    Gwen, I suggest adding this check (reading the checkpoint file and ensuring that the entries related to the deleted topic are gone) to the DeleteTopicTest. 
    
    Rest of the patch looks good. We can check it in after this change


- Neha Narkhede


On Dec. 26, 2014, 9:58 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29210/
> -----------------------------------------------------------
> 
> (Updated Dec. 26, 2014, 9:58 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1819
>     https://issues.apache.org/jira/browse/KAFKA-1819
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> added locking
> 
> 
> improved tests per Joel and Neha's suggestions
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
>   core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
>   core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
>   core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 
> 
> Diff: https://reviews.apache.org/r/29210/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/#review68107
-----------------------------------------------------------


A couple of minor comments. Neither of these is a blocker for 0.8.2. So we just need to patch trunk.

I saw the following unit test failure. Could we reduce the amount of data used in writeDups()?

kafka.admin.DeleteTopicTest > testDeleteTopicWithCleaner FAILED
    java.lang.OutOfMemoryError: Java heap space
        at java.nio.HeapByteBuffer.<init>(HeapByteBuffer.java:39)
        at java.nio.ByteBuffer.allocate(ByteBuffer.java:312)
        at kafka.log.SkimpyOffsetMap.<init>(OffsetMap.scala:42)
        at kafka.log.LogCleaner$CleanerThread.<init>(LogCleaner.scala:177)
        at kafka.log.LogCleaner$$anonfun$1.apply(LogCleaner.scala:86)
        at kafka.log.LogCleaner$$anonfun$1.apply(LogCleaner.scala:86)
        at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
        at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
        at scala.collection.immutable.Range.foreach(Range.scala:141)
        at scala.collection.TraversableLike$class.map(TraversableLike.scala:244)
        at scala.collection.AbstractTraversable.map(Traversable.scala:105)
        at kafka.log.LogCleaner.<init>(LogCleaner.scala:86)
        at kafka.log.LogManager.<init>(LogManager.scala:64)
        at kafka.server.KafkaServer.createLogManager(KafkaServer.scala:344)
        at kafka.server.KafkaServer.startup(KafkaServer.scala:89)
        at kafka.utils.TestUtils$.createServer(TestUtils.scala:134)
        at kafka.admin.DeleteTopicTest$$anonfun$10.apply(DeleteTopicTest.scala:272)
        at kafka.admin.DeleteTopicTest$$anonfun$10.apply(DeleteTopicTest.scala:272)
        at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
        at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
        at scala.collection.immutable.List.foreach(List.scala:318)
        at scala.collection.TraversableLike$class.map(TraversableLike.scala:244)
        at scala.collection.AbstractTraversable.map(Traversable.scala:105)
        at kafka.admin.DeleteTopicTest.createTestTopicAndCluster(DeleteTopicTest.scala:272)
        at kafka.admin.DeleteTopicTest.testDeleteTopicWithCleaner(DeleteTopicTest.scala:241)


core/src/main/scala/kafka/log/LogManager.scala
<https://reviews.apache.org/r/29210/#comment112274>

    Instead of making it public, would it be better to expose a awaitCleaned() method?


- Jun Rao


On Jan. 13, 2015, 1:01 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29210/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2015, 1:01 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1819
>     https://issues.apache.org/jira/browse/KAFKA-1819
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> added locking
> 
> 
> improved tests per Joel and Neha's suggestions
> 
> 
> added cleaner test to DeleteTopicTest
> 
> 
> Fixes to DeleteTopicTest: clean up servers after cleaner test and move cleaner verification to the validation function
> 
> 
> minor fixes suggested by Joel
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
>   core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
>   core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 
> 
> Diff: https://reviews.apache.org/r/29210/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/
-----------------------------------------------------------

(Updated Jan. 13, 2015, 1:01 a.m.)


Review request for kafka.


Bugs: KAFKA-1819
    https://issues.apache.org/jira/browse/KAFKA-1819


Repository: kafka


Description (updated)
-------

added locking


improved tests per Joel and Neha's suggestions


added cleaner test to DeleteTopicTest


Fixes to DeleteTopicTest: clean up servers after cleaner test and move cleaner verification to the validation function


minor fixes suggested by Joel


Diffs (updated)
-----

  core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
  core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
  core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
  core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 

Diff: https://reviews.apache.org/r/29210/diff/


Testing
-------


Thanks,

Gwen Shapira

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Gwen Shapira <gs...@cloudera.com>.

> On Jan. 12, 2015, 10:53 p.m., Joel Koshy wrote:
> > core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala, line 251
> > <https://reviews.apache.org/r/29210/diff/5/?file=816889#file816889line251>
> >
> >     Rather than an arbitrary sleep can we just use waitUntilTrue and the condition would be that the cleaner checkpoint file contains the topic.

Instead of waitUntilTrue, I'm calling LogCleaner.awaitCleaned, to avoid replicating the logic. 
This required exposing LogCleaner in LogManager.


- Gwen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/#review67717
-----------------------------------------------------------


On Jan. 12, 2015, 7:17 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29210/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 7:17 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1819
>     https://issues.apache.org/jira/browse/KAFKA-1819
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> added locking
> 
> 
> improved tests per Joel and Neha's suggestions
> 
> 
> added cleaner test to DeleteTopicTest
> 
> 
> Fixes to DeleteTopicTest: clean up servers after cleaner test and move cleaner verification to the validation function
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
>   core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
>   core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 
> 
> Diff: https://reviews.apache.org/r/29210/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Neha Narkhede <ne...@gmail.com>.

> On Jan. 12, 2015, 10:53 p.m., Joel Koshy wrote:
> > core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala, line 68
> > <https://reviews.apache.org/r/29210/diff/5/?file=816890#file816890line68>
> >
> >     Actually, now that you have the check above in DeleteTopicTest, do you think it is necessary to have this?
> 
> Gwen Shapira wrote:
>     Both checks exercise the same code path, so I don't think we need both. Less code to maintain is always better, IMO.
>     
>     Since Neha asked to add the test in DeleteTopicTest, I want to make sure that she's ok with removing it from LogCleanerIntegrationTest. Maybe I'm missing a good reason to have both.

I'm ok with removing the changes to LogCleanerIntegrationTest


- Neha


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/#review67717
-----------------------------------------------------------


On Jan. 13, 2015, 1:01 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29210/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2015, 1:01 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1819
>     https://issues.apache.org/jira/browse/KAFKA-1819
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> added locking
> 
> 
> improved tests per Joel and Neha's suggestions
> 
> 
> added cleaner test to DeleteTopicTest
> 
> 
> Fixes to DeleteTopicTest: clean up servers after cleaner test and move cleaner verification to the validation function
> 
> 
> minor fixes suggested by Joel
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
>   core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
>   core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 
> 
> Diff: https://reviews.apache.org/r/29210/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Gwen Shapira <gs...@cloudera.com>.

> On Jan. 12, 2015, 10:53 p.m., Joel Koshy wrote:
> > core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala, line 68
> > <https://reviews.apache.org/r/29210/diff/5/?file=816890#file816890line68>
> >
> >     Actually, now that you have the check above in DeleteTopicTest, do you think it is necessary to have this?

Both checks exercise the same code path, so I don't think we need both. Less code to maintain is always better, IMO.

Since Neha asked to add the test in DeleteTopicTest, I want to make sure that she's ok with removing it from LogCleanerIntegrationTest. Maybe I'm missing a good reason to have both.


- Gwen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/#review67717
-----------------------------------------------------------


On Jan. 12, 2015, 7:17 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29210/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 7:17 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1819
>     https://issues.apache.org/jira/browse/KAFKA-1819
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> added locking
> 
> 
> improved tests per Joel and Neha's suggestions
> 
> 
> added cleaner test to DeleteTopicTest
> 
> 
> Fixes to DeleteTopicTest: clean up servers after cleaner test and move cleaner verification to the validation function
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
>   core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
>   core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 
> 
> Diff: https://reviews.apache.org/r/29210/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/#review67717
-----------------------------------------------------------


Minor comments


core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala
<https://reviews.apache.org/r/29210/#comment111813>

    unused



core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala
<https://reviews.apache.org/r/29210/#comment111807>

    Rather than an arbitrary sleep can we just use waitUntilTrue and the condition would be that the cleaner checkpoint file contains the topic.



core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala
<https://reviews.apache.org/r/29210/#comment111809>

    Actually, now that you have the check above in DeleteTopicTest, do you think it is necessary to have this?


- Joel Koshy


On Jan. 12, 2015, 7:17 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29210/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 7:17 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1819
>     https://issues.apache.org/jira/browse/KAFKA-1819
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> added locking
> 
> 
> improved tests per Joel and Neha's suggestions
> 
> 
> added cleaner test to DeleteTopicTest
> 
> 
> Fixes to DeleteTopicTest: clean up servers after cleaner test and move cleaner verification to the validation function
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
>   core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
>   core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 
> 
> Diff: https://reviews.apache.org/r/29210/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/
-----------------------------------------------------------

(Updated Jan. 12, 2015, 7:17 p.m.)


Review request for kafka.


Bugs: KAFKA-1819
    https://issues.apache.org/jira/browse/KAFKA-1819


Repository: kafka


Description (updated)
-------

added locking


improved tests per Joel and Neha's suggestions


added cleaner test to DeleteTopicTest


Fixes to DeleteTopicTest: clean up servers after cleaner test and move cleaner verification to the validation function


Diffs (updated)
-----

  core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
  core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
  core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
  core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 

Diff: https://reviews.apache.org/r/29210/diff/


Testing
-------


Thanks,

Gwen Shapira

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/
-----------------------------------------------------------

(Updated Jan. 12, 2015, 6:34 p.m.)


Review request for kafka.


Bugs: KAFKA-1819
    https://issues.apache.org/jira/browse/KAFKA-1819


Repository: kafka


Description (updated)
-------

kafka-1797; (addressing Manikumar Reddy's comment) add the serializer/deserializer api to the new java client; patched by Jun Rao; reviewed by Manikumar Reddy and Neha Narkhede


kafka-1851; OffsetFetchRequest returns extra partitions when input only contains unknown partitions; patched by Jun Rao; reviewed by Neha Narkhede


first pass at log clean fix


added locking


improved tests per Joel and Neha's suggestions


added cleaner test to DeleteTopicTest


Fixes to DeleteTopicTest: clean up servers after cleaner test and move cleaner verification to the validation function


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/common/serialization/StringDeserializer.java a3b3700a1e0716643761d7032bd32bce839d3065 
  clients/src/main/java/org/apache/kafka/common/serialization/StringSerializer.java 02db47f8736988343dd293fc3da03751f78a3b5c 
  clients/src/test/java/org/apache/kafka/common/serialization/SerializationTest.java d550a3137c066abb5e2984ac6245574832929ff8 
  core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
  core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
  core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
  core/src/main/scala/kafka/network/SocketServer.scala e451592fe358158548117f47a80e807007dd8b98 
  core/src/main/scala/kafka/server/KafkaApis.scala 2a1c0326b6e6966d8b8254bd6a1cb83ad98a3b80 
  core/src/main/scala/kafka/server/KafkaServer.scala 1bf7d10cef23a77e716666eb16bf6d0e68bc4ebe 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
  core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 
  core/src/test/scala/unit/kafka/network/SocketServerTest.scala 5f4d85254c384dcc27a5a84f0836ea225d3a901a 
  core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 8c5364fa97da1be09973c176d1baeb339455d319 

Diff: https://reviews.apache.org/r/29210/diff/


Testing
-------


Thanks,

Gwen Shapira

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/#review66733
-----------------------------------------------------------



core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala
<https://reviews.apache.org/r/29210/#comment110339>

    suggest moving this logic to verifyTopicDeletion. It is one of the important validity checks that checks if there are any deleted topics in the cleaner checkpoint.


- Neha Narkhede


On Dec. 31, 2014, 12:01 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29210/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2014, 12:01 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1819
>     https://issues.apache.org/jira/browse/KAFKA-1819
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> added locking
> 
> 
> improved tests per Joel and Neha's suggestions
> 
> 
> added cleaner test to DeleteTopicTest
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
>   core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
>   core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 
> 
> Diff: https://reviews.apache.org/r/29210/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/
-----------------------------------------------------------

(Updated Dec. 31, 2014, 12:01 a.m.)


Review request for kafka.


Bugs: KAFKA-1819
    https://issues.apache.org/jira/browse/KAFKA-1819


Repository: kafka


Description (updated)
-------

added locking


improved tests per Joel and Neha's suggestions


added cleaner test to DeleteTopicTest


Diffs (updated)
-----

  core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
  core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
  core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
  core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 

Diff: https://reviews.apache.org/r/29210/diff/


Testing
-------


Thanks,

Gwen Shapira

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/
-----------------------------------------------------------

(Updated Dec. 26, 2014, 9:58 p.m.)


Review request for kafka.


Bugs: KAFKA-1819
    https://issues.apache.org/jira/browse/KAFKA-1819


Repository: kafka


Description (updated)
-------

added locking


improved tests per Joel and Neha's suggestions


Diffs (updated)
-----

  core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
  core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
  core/src/main/scala/kafka/log/LogManager.scala 4ebaae00ca4b80bf15c7930bae2011d98bbec053 
  core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 5bfa764638e92f217d0ff7108ec8f53193c22978 

Diff: https://reviews.apache.org/r/29210/diff/


Testing
-------


Thanks,

Gwen Shapira

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Neha Narkhede <ne...@gmail.com>.

> On Dec. 19, 2014, 1:23 a.m., Neha Narkhede wrote:
> > core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala, line 231
> > <https://reviews.apache.org/r/29210/diff/1/?file=796108#file796108line231>
> >
> >     Since the bug is about entries related to deleted topics, it will be good to add that verification step to all tests in DeleteTopicTest.
> 
> Gwen Shapira wrote:
>     I added the "delete.topic.enable" to createTestTopicAndCluster(), which is used by all tests in DeleteTopicTest. This exercises the part of the code-path where we abort and checkpoint the cleaner.
>     
>     However, this does not provide any verification that the cleaner checkpoint file was correctly updated. I wanted to add that, but it looks like getting the content of the checkpoint file from the information available at the delete topic tests will require quite intrusive modification to unrelated parts of the code (the cleaner being private to the log, for example). So I left this part out, and did some manual testing instead.

Gwen, the logic for reading/writing an offset checkpoint is encapsulated in OffsetCheckpoint.scala. I was thinking we should add a helper method to read cleaner checkpoints from there and see if the deleted topic exists or not. I do think that no matter what other testing we do, we should definitely add that as it is meant to be a sanity check to verify that all state that is associated with the deleted topic is truly gone.


- Neha


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/#review65578
-----------------------------------------------------------


On Dec. 18, 2014, 6:59 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29210/
> -----------------------------------------------------------
> 
> (Updated Dec. 18, 2014, 6:59 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1819
>     https://issues.apache.org/jira/browse/KAFKA-1819
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> added locking
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
>   core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
>   core/src/main/scala/kafka/log/LogManager.scala 4d2924d04bc4bd62413edb0ee2d4aaf3c0052867 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
> 
> Diff: https://reviews.apache.org/r/29210/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Gwen Shapira <gs...@cloudera.com>.

> On Dec. 19, 2014, 1:23 a.m., Neha Narkhede wrote:
> > core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala, line 231
> > <https://reviews.apache.org/r/29210/diff/1/?file=796108#file796108line231>
> >
> >     Since the bug is about entries related to deleted topics, it will be good to add that verification step to all tests in DeleteTopicTest.

I added the "delete.topic.enable" to createTestTopicAndCluster(), which is used by all tests in DeleteTopicTest. This exercises the part of the code-path where we abort and checkpoint the cleaner.

However, this does not provide any verification that the cleaner checkpoint file was correctly updated. I wanted to add that, but it looks like getting the content of the checkpoint file from the information available at the delete topic tests will require quite intrusive modification to unrelated parts of the code (the cleaner being private to the log, for example). So I left this part out, and did some manual testing instead.


- Gwen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/#review65578
-----------------------------------------------------------


On Dec. 18, 2014, 6:59 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29210/
> -----------------------------------------------------------
> 
> (Updated Dec. 18, 2014, 6:59 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1819
>     https://issues.apache.org/jira/browse/KAFKA-1819
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> added locking
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
>   core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
>   core/src/main/scala/kafka/log/LogManager.scala 4d2924d04bc4bd62413edb0ee2d4aaf3c0052867 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
> 
> Diff: https://reviews.apache.org/r/29210/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/#review65578
-----------------------------------------------------------


Overall, looks good. Have one suggestion below.


core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala
<https://reviews.apache.org/r/29210/#comment108824>

    Since the bug is about entries related to deleted topics, it will be good to add that verification step to all tests in DeleteTopicTest.


- Neha Narkhede


On Dec. 18, 2014, 6:59 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29210/
> -----------------------------------------------------------
> 
> (Updated Dec. 18, 2014, 6:59 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1819
>     https://issues.apache.org/jira/browse/KAFKA-1819
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> added locking
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
>   core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
>   core/src/main/scala/kafka/log/LogManager.scala 4d2924d04bc4bd62413edb0ee2d4aaf3c0052867 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
> 
> Diff: https://reviews.apache.org/r/29210/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 29210: Patch for KAFKA-1819

Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29210/#review65866
-----------------------------------------------------------



core/src/main/scala/kafka/log/LogCleaner.scala
<https://reviews.apache.org/r/29210/#comment109115>

    * space after comma
    * For boolean or None arguments I personally prefer explicitly naming the argument - i.e., update = None
    * (Alternatively we can just use have the argument default to None)



core/src/main/scala/kafka/log/LogCleanerManager.scala
<https://reviews.apache.org/r/29210/#comment109118>

    We can probably drop the comment as it is understood throughout the class



core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala
<https://reviews.apache.org/r/29210/#comment109125>

    I think it would suffice to exercise this logic as part of LogCleanerIntegrationTest. We can just "simulate" deletion by removing the entry from the logs map. Although the access methods for the cleaner checkpoints are inaccessible there, we can directly read the checkpoint file. LogManagerTest.verifyCheckpointRecovery has a similar example. Let me know if that works or not.


- Joel Koshy


On Dec. 18, 2014, 6:59 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29210/
> -----------------------------------------------------------
> 
> (Updated Dec. 18, 2014, 6:59 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1819
>     https://issues.apache.org/jira/browse/KAFKA-1819
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> added locking
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/log/LogCleaner.scala f8fcb843c80eec3cf3c931df6bb472c019305253 
>   core/src/main/scala/kafka/log/LogCleanerManager.scala bcfef77ed53f94017c06a884e4db14531774a0a2 
>   core/src/main/scala/kafka/log/LogManager.scala 4d2924d04bc4bd62413edb0ee2d4aaf3c0052867 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
> 
> Diff: https://reviews.apache.org/r/29210/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>