----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33168/#review80543 -----------------------------------------------------------
Thanks for the patch. Great finding and very comprehensive tests! Just a couple of minor comments below. core/src/main/scala/kafka/log/Log.scala <https://reviews.apache.org/r/33168/#comment130557> It's not clear what "this operation" is. It seems to refer to replaceSegments(), bit replaceSegments() doesn't create new suffix .cleaned. So, it probably should refer to the cleaning process. core/src/main/scala/kafka/log/Log.scala <https://reviews.apache.org/r/33168/#comment130550> What happens if the broker crashes just before the swapping in line 805. Then, when calling replaceSegments() during broker restart, you will have old segments already with suffix .delete. In asyncDeleteSegment(), we will then try to rename the file by adding another .delete suffix. The deletion will probaby still work, but this will be a bit wierd. - Jun Rao On April 15, 2015, 9:44 a.m., Rajini Sivaram wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33168/ > ----------------------------------------------------------- > > (Updated April 15, 2015, 9:44 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2118 > https://issues.apache.org/jira/browse/KAFKA-2118 > > > Repository: kafka > > > Description > ------- > > Patch for KAFKA-2118: Fix recovery of cleaned segments after broker crash > > > Diffs > ----- > > core/src/main/scala/kafka/log/Log.scala > 5563f2de8113a0ece8929bec9c75dbf892abbb66 > core/src/test/scala/unit/kafka/log/CleanerTest.scala > 9792ed689033dbd4ad99809a4e566136d2b9fadf > > Diff: https://reviews.apache.org/r/33168/diff/ > > > Testing > ------- > > > Thanks, > > Rajini Sivaram > >
