You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jaikiran Pai <ja...@gmail.com> on 2015/01/09 07:44:53 UTC
Review Request 29755: Patch for KAFKA-1853
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29755/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1853
https://issues.apache.org/jira/browse/KAFKA-1853
Repository: kafka
Description
-------
Fixes file leaks caused by unsuccessful file rename attempt in LogSegment
Diffs
-----
core/src/main/scala/kafka/log/FileMessageSet.scala b2652ddbe2f857028d5980e29a008b2c614694a3
core/src/test/scala/unit/kafka/log/LogSegmentTest.scala 03fb3512c4a4450eac83d4cd4b0919baeaa22942
Diff: https://reviews.apache.org/r/29755/diff/
Testing
-------
Thanks,
Jaikiran Pai
Re: Review Request 29755: Patch for KAFKA-1853
Posted by Jaikiran Pai <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29755/#review67382
-----------------------------------------------------------
This is my very first attempt at Scala, so feel free to let me know if anything looks incorrect. I've added a test to ensure that the change at least works as per my expectation.
- Jaikiran Pai
On Jan. 9, 2015, 6:44 a.m., Jaikiran Pai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29755/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2015, 6:44 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1853
> https://issues.apache.org/jira/browse/KAFKA-1853
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fixes file leaks caused by unsuccessful file rename attempt in LogSegment
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/FileMessageSet.scala b2652ddbe2f857028d5980e29a008b2c614694a3
> core/src/test/scala/unit/kafka/log/LogSegmentTest.scala 03fb3512c4a4450eac83d4cd4b0919baeaa22942
>
> Diff: https://reviews.apache.org/r/29755/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jaikiran Pai
>
>
Re: Review Request 29755: Patch for KAFKA-1853
Posted by Jaikiran Pai <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29755/
-----------------------------------------------------------
(Updated Jan. 26, 2015, 2:11 p.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 (updated)
-----
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
Re: Review Request 29755: Patch for KAFKA-1853
Posted by Jaikiran Pai <ja...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 29755: Patch for KAFKA-1853
Posted by Jaikiran Pai <ja...@gmail.com>.
> 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
>
>
Re: Review Request 29755: Patch for KAFKA-1853
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29755/#review69529
-----------------------------------------------------------
core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/29755/#comment114225>
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?
- Jay Kreps
On Jan. 24, 2015, 5:18 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:18 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
>
>
Re: Review Request 29755: Patch for KAFKA-1853
Posted by Jaikiran Pai <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29755/
-----------------------------------------------------------
(Updated Jan. 24, 2015, 5:18 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 (updated)
-----
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
Re: Review Request 29755: Patch for KAFKA-1853
Posted by Jaikiran Pai <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29755/
-----------------------------------------------------------
(Updated Jan. 20, 2015, 4:35 p.m.)
Review request for kafka.
Bugs: KAFKA-1853
https://issues.apache.org/jira/browse/KAFKA-1853
Repository: kafka
Description (updated)
-------
KAFKA-1853 Prevent leaking open file resources when renaming of the LogSegment fails
Diffs (updated)
-----
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
Re: Review Request 29755: Patch for KAFKA-1853
Posted by Jaikiran Pai <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29755/
-----------------------------------------------------------
(Updated Jan. 10, 2015, 1:37 p.m.)
Review request for kafka.
Bugs: KAFKA-1853
https://issues.apache.org/jira/browse/KAFKA-1853
Repository: kafka
Description
-------
KAFKA-1853 Fix potential leak of open LogSegment files during deletion of expired LogSegments. Also fixes the issue of leaving FileMessageSet in a invalid state in such cases
Diffs
-----
core/src/main/scala/kafka/log/FileMessageSet.scala b2652ddbe2f857028d5980e29a008b2c614694a3
core/src/main/scala/kafka/log/Log.scala 024506cd00556a0037c0b3b6b603da32968b69ab
Diff: https://reviews.apache.org/r/29755/diff/
Testing (updated)
-------
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
Re: Review Request 29755: Patch for KAFKA-1853
Posted by Jaikiran Pai <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29755/
-----------------------------------------------------------
(Updated Jan. 10, 2015, 1:34 p.m.)
Review request for kafka.
Bugs: KAFKA-1853
https://issues.apache.org/jira/browse/KAFKA-1853
Repository: kafka
Description (updated)
-------
KAFKA-1853 Fix potential leak of open LogSegment files during deletion of expired LogSegments. Also fixes the issue of leaving FileMessageSet in a invalid state in such cases
Diffs (updated)
-----
core/src/main/scala/kafka/log/FileMessageSet.scala b2652ddbe2f857028d5980e29a008b2c614694a3
core/src/main/scala/kafka/log/Log.scala 024506cd00556a0037c0b3b6b603da32968b69ab
Diff: https://reviews.apache.org/r/29755/diff/
Testing
-------
Thanks,
Jaikiran Pai