> On Jan. 24, 2015, 5:43 a.m., Jay Kreps wrote: > > core/src/main/scala/kafka/log/Log.scala, line 751 > > <https://reviews.apache.org/r/29755/diff/4/?file=833256#file833256line751> > > > > Seems like in the case of the exception we actually forcefully delete, > > but then after deleting schedule another one. > > > > Should the scheduler action be inside the try block so it only occurs > > if the rename succeeds?
Sorry, I missed a return statement in the catch block. Fixed it now and updated the patch. Thanks Jay! - Jaikiran ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29755/#review69529 ----------------------------------------------------------- On Jan. 24, 2015, 5:51 a.m., Jaikiran Pai wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29755/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2015, 5:51 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1853 > https://issues.apache.org/jira/browse/KAFKA-1853 > > > Repository: kafka > > > Description > ------- > > KAFKA-1853 Prevent leaking open file resources when renaming of the > LogSegment fails > > > Diffs > ----- > > core/src/main/scala/kafka/log/FileMessageSet.scala > b2652ddbe2f857028d5980e29a008b2c614694a3 > core/src/main/scala/kafka/log/Log.scala > 846023bb98d0fa0603016466360c97071ac935ea > core/src/main/scala/kafka/log/OffsetIndex.scala > 1c4c7bd89e19ea942cf1d01eafe502129e97f535 > > Diff: https://reviews.apache.org/r/29755/diff/ > > > Testing > ------- > > Have run the existing tests in LogManagerTest (which includes a test for > cleaning of expired LogSegments) and those have passed with this change. I > did give a thought of trying to replicate a failed rename scenario and then > to ensure that we don't leak resources anymore, but that's not > straightforward to do in the tests, so haven't added any new tests. > > > Thanks, > > Jaikiran Pai > >
