You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jun Rao <ju...@gmail.com> on 2014/01/04 17:37:22 UTC
Re: Review Request 15674: rename LogCleanerStates to LogCleanerManagers;
optimize truncateTo to only pause cleaning if the truncated offset is not on
the active segment
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15674/
-----------------------------------------------------------
(Updated Jan. 4, 2014, 4:37 p.m.)
Review request for kafka.
Summary (updated)
-----------------
rename LogCleanerStates to LogCleanerManagers; optimize truncateTo to only pause cleaning if the truncated offset is not on the active segment
Bugs: KAFKA-1074
https://issues.apache.org/jira/browse/KAFKA-1074
Repository: kafka
Description (updated)
-------
rename LogCleanerStates to LogCleanerManagers; optimize truncateTo to only pause cleaning if the truncated offset is not on the active segment
add test for aborting during cleaning
checkpoint recovery points in truncateFullyAndStartAt()
move cleaning states and locks to a separate class
minor fix 2
minor fix
remove unused var and exception
support pause/resume log clean and remove OptimisticLockFailureException
kafka-1074; minor fix3
kafka-1074; minor fix2
kafka-1074; minor fix
kafka-1074; better synchronization with log cleaner
kafka-1074; fix 4
kafka-1074; fix 3
kafka-1074; fix 2
kafka-1074
Diffs (updated)
-----
core/src/main/scala/kafka/cluster/Partition.scala 5c9307d71641ccc6c09a54b69d5aa2b4bc2a4cde
core/src/main/scala/kafka/common/OptimisticLockFailureException.scala 0e69110e71667ad75a460534f4422a7b6ec1cdc6
core/src/main/scala/kafka/common/ThreadShutdownException.scala PRE-CREATION
core/src/main/scala/kafka/log/Log.scala beda421b558544196379bd9ab7855cea7614e8e3
core/src/main/scala/kafka/log/LogCleaner.scala ccde2abd99d2204775e4d3e9836aca34eb6747a3
core/src/main/scala/kafka/log/LogManager.scala 81be88aa618ed5614703d45a0556b77c97290085
core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 715845b167c44268bd7e4b76dfb69199bfb2fad4
core/src/main/scala/kafka/server/ReplicaManager.scala 242c18d47828b7c5e6b1fc219a0f1199fb1f9512
core/src/test/scala/unit/kafka/admin/AdminTest.scala 52d35a3ccc86a66acb57c1138d4f2fea8f4ca4b0
core/src/test/scala/unit/kafka/log/CleanerTest.scala 5a312bf0803c1df4636e2e64ba83036a4e8e92dd
Diff: https://reviews.apache.org/r/15674/diff/
Testing
-------
Thanks,
Jun Rao
Re: Review Request 15674: move LogCleanerManager to a separate file and
improve unit test
Posted by Jun Rao <ju...@gmail.com>.
> On Jan. 6, 2014, 9:18 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/log/LogCleaner.scala, line 161
> > <https://reviews.apache.org/r/15674/diff/6/?file=415674#file415674line161>
> >
> > the method description says it should block until the cleaner has processed upto the given offset. But it doesn't look like the offset parameter is used in any way?
This is an existing problem. Will file a separate jira to have it addressed.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15674/#review31247
-----------------------------------------------------------
On Jan. 7, 2014, 6:51 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15674/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2014, 6:51 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1074
> https://issues.apache.org/jira/browse/KAFKA-1074
>
>
> Repository: kafka
>
>
> Description
> -------
>
> move LogCleanerManager to a separate file and improve unit test
>
>
> rename LogCleanerStates to LogCleanerManagers; optimize truncateTo to only pause cleaning if the truncated offset is not on the active segment
>
>
> add test for aborting during cleaning
>
>
> checkpoint recovery points in truncateFullyAndStartAt()
>
>
> move cleaning states and locks to a separate class
>
>
> minor fix 2
>
>
> minor fix
>
>
> remove unused var and exception
>
>
> support pause/resume log clean and remove OptimisticLockFailureException
>
>
> kafka-1074; minor fix3
>
>
> kafka-1074; minor fix2
>
>
> kafka-1074; minor fix
>
>
> kafka-1074; better synchronization with log cleaner
>
>
> kafka-1074; fix 4
>
>
> kafka-1074; fix 3
>
>
> kafka-1074; fix 2
>
>
> kafka-1074
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 5c9307d71641ccc6c09a54b69d5aa2b4bc2a4cde
> core/src/main/scala/kafka/common/LogCleaningAbortedException.scala PRE-CREATION
> core/src/main/scala/kafka/common/OptimisticLockFailureException.scala 0e69110e71667ad75a460534f4422a7b6ec1cdc6
> core/src/main/scala/kafka/log/Log.scala beda421b558544196379bd9ab7855cea7614e8e3
> core/src/main/scala/kafka/log/LogCleaner.scala ccde2abd99d2204775e4d3e9836aca34eb6747a3
> core/src/main/scala/kafka/log/LogCleanerManager.scala PRE-CREATION
> core/src/main/scala/kafka/log/LogManager.scala 81be88aa618ed5614703d45a0556b77c97290085
> core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 715845b167c44268bd7e4b76dfb69199bfb2fad4
> core/src/main/scala/kafka/server/ReplicaManager.scala 242c18d47828b7c5e6b1fc219a0f1199fb1f9512
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 52d35a3ccc86a66acb57c1138d4f2fea8f4ca4b0
> core/src/test/scala/unit/kafka/log/CleanerTest.scala 5a312bf0803c1df4636e2e64ba83036a4e8e92dd
>
> Diff: https://reviews.apache.org/r/15674/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 15674: rename LogCleanerStates to LogCleanerManagers;
optimize truncateTo to only pause cleaning if the truncated offset is not on
the active segment
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15674/#review31247
-----------------------------------------------------------
Overall, looks good. Few questions and minor comments
core/src/main/scala/kafka/log/LogCleaner.scala
<https://reviews.apache.org/r/15674/#comment59648>
the method description says it should block until the cleaner has processed upto the given offset. But it doesn't look like the offset parameter is used in any way?
core/src/main/scala/kafka/log/LogCleaner.scala
<https://reviews.apache.org/r/15674/#comment59650>
it is worth placing the LogCleanerManager and the Cleaner in separate class files. Makes the code easier to read
core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/15674/#comment59646>
This check is good. In addition to this, let's also validate that the only brokers that have the partition directory are the brokers in the reassigned replica list.
- Neha Narkhede
On Jan. 4, 2014, 4:37 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15674/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2014, 4:37 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1074
> https://issues.apache.org/jira/browse/KAFKA-1074
>
>
> Repository: kafka
>
>
> Description
> -------
>
> rename LogCleanerStates to LogCleanerManagers; optimize truncateTo to only pause cleaning if the truncated offset is not on the active segment
>
>
> add test for aborting during cleaning
>
>
> checkpoint recovery points in truncateFullyAndStartAt()
>
>
> move cleaning states and locks to a separate class
>
>
> minor fix 2
>
>
> minor fix
>
>
> remove unused var and exception
>
>
> support pause/resume log clean and remove OptimisticLockFailureException
>
>
> kafka-1074; minor fix3
>
>
> kafka-1074; minor fix2
>
>
> kafka-1074; minor fix
>
>
> kafka-1074; better synchronization with log cleaner
>
>
> kafka-1074; fix 4
>
>
> kafka-1074; fix 3
>
>
> kafka-1074; fix 2
>
>
> kafka-1074
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 5c9307d71641ccc6c09a54b69d5aa2b4bc2a4cde
> core/src/main/scala/kafka/common/OptimisticLockFailureException.scala 0e69110e71667ad75a460534f4422a7b6ec1cdc6
> core/src/main/scala/kafka/common/ThreadShutdownException.scala PRE-CREATION
> core/src/main/scala/kafka/log/Log.scala beda421b558544196379bd9ab7855cea7614e8e3
> core/src/main/scala/kafka/log/LogCleaner.scala ccde2abd99d2204775e4d3e9836aca34eb6747a3
> core/src/main/scala/kafka/log/LogManager.scala 81be88aa618ed5614703d45a0556b77c97290085
> core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 715845b167c44268bd7e4b76dfb69199bfb2fad4
> core/src/main/scala/kafka/server/ReplicaManager.scala 242c18d47828b7c5e6b1fc219a0f1199fb1f9512
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 52d35a3ccc86a66acb57c1138d4f2fea8f4ca4b0
> core/src/test/scala/unit/kafka/log/CleanerTest.scala 5a312bf0803c1df4636e2e64ba83036a4e8e92dd
>
> Diff: https://reviews.apache.org/r/15674/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 15674: move LogCleanerManager to a separate file and
improve unit test
Posted by Jun Rao <ju...@gmail.com>.
> On Jan. 7, 2014, 11:42 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/common/OptimisticLockFailureException.scala, line 23
> > <https://reviews.apache.org/r/15674/diff/6-7/?file=415671#file415671line23>
> >
> > Is this change intended?
This is a diff btw v6 and v7, but is not in v7 itself.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15674/#review31322
-----------------------------------------------------------
On Jan. 7, 2014, 6:51 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15674/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2014, 6:51 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1074
> https://issues.apache.org/jira/browse/KAFKA-1074
>
>
> Repository: kafka
>
>
> Description
> -------
>
> move LogCleanerManager to a separate file and improve unit test
>
>
> rename LogCleanerStates to LogCleanerManagers; optimize truncateTo to only pause cleaning if the truncated offset is not on the active segment
>
>
> add test for aborting during cleaning
>
>
> checkpoint recovery points in truncateFullyAndStartAt()
>
>
> move cleaning states and locks to a separate class
>
>
> minor fix 2
>
>
> minor fix
>
>
> remove unused var and exception
>
>
> support pause/resume log clean and remove OptimisticLockFailureException
>
>
> kafka-1074; minor fix3
>
>
> kafka-1074; minor fix2
>
>
> kafka-1074; minor fix
>
>
> kafka-1074; better synchronization with log cleaner
>
>
> kafka-1074; fix 4
>
>
> kafka-1074; fix 3
>
>
> kafka-1074; fix 2
>
>
> kafka-1074
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 5c9307d71641ccc6c09a54b69d5aa2b4bc2a4cde
> core/src/main/scala/kafka/common/LogCleaningAbortedException.scala PRE-CREATION
> core/src/main/scala/kafka/common/OptimisticLockFailureException.scala 0e69110e71667ad75a460534f4422a7b6ec1cdc6
> core/src/main/scala/kafka/log/Log.scala beda421b558544196379bd9ab7855cea7614e8e3
> core/src/main/scala/kafka/log/LogCleaner.scala ccde2abd99d2204775e4d3e9836aca34eb6747a3
> core/src/main/scala/kafka/log/LogCleanerManager.scala PRE-CREATION
> core/src/main/scala/kafka/log/LogManager.scala 81be88aa618ed5614703d45a0556b77c97290085
> core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 715845b167c44268bd7e4b76dfb69199bfb2fad4
> core/src/main/scala/kafka/server/ReplicaManager.scala 242c18d47828b7c5e6b1fc219a0f1199fb1f9512
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 52d35a3ccc86a66acb57c1138d4f2fea8f4ca4b0
> core/src/test/scala/unit/kafka/log/CleanerTest.scala 5a312bf0803c1df4636e2e64ba83036a4e8e92dd
>
> Diff: https://reviews.apache.org/r/15674/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 15674: move LogCleanerManager to a separate file and
improve unit test
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15674/#review31322
-----------------------------------------------------------
Ship it!
Other than the question below, I think this patch looks good.
core/src/main/scala/kafka/common/OptimisticLockFailureException.scala
<https://reviews.apache.org/r/15674/#comment59786>
Is this change intended?
- Neha Narkhede
On Jan. 7, 2014, 6:51 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15674/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2014, 6:51 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1074
> https://issues.apache.org/jira/browse/KAFKA-1074
>
>
> Repository: kafka
>
>
> Description
> -------
>
> move LogCleanerManager to a separate file and improve unit test
>
>
> rename LogCleanerStates to LogCleanerManagers; optimize truncateTo to only pause cleaning if the truncated offset is not on the active segment
>
>
> add test for aborting during cleaning
>
>
> checkpoint recovery points in truncateFullyAndStartAt()
>
>
> move cleaning states and locks to a separate class
>
>
> minor fix 2
>
>
> minor fix
>
>
> remove unused var and exception
>
>
> support pause/resume log clean and remove OptimisticLockFailureException
>
>
> kafka-1074; minor fix3
>
>
> kafka-1074; minor fix2
>
>
> kafka-1074; minor fix
>
>
> kafka-1074; better synchronization with log cleaner
>
>
> kafka-1074; fix 4
>
>
> kafka-1074; fix 3
>
>
> kafka-1074; fix 2
>
>
> kafka-1074
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala 5c9307d71641ccc6c09a54b69d5aa2b4bc2a4cde
> core/src/main/scala/kafka/common/LogCleaningAbortedException.scala PRE-CREATION
> core/src/main/scala/kafka/common/OptimisticLockFailureException.scala 0e69110e71667ad75a460534f4422a7b6ec1cdc6
> core/src/main/scala/kafka/log/Log.scala beda421b558544196379bd9ab7855cea7614e8e3
> core/src/main/scala/kafka/log/LogCleaner.scala ccde2abd99d2204775e4d3e9836aca34eb6747a3
> core/src/main/scala/kafka/log/LogCleanerManager.scala PRE-CREATION
> core/src/main/scala/kafka/log/LogManager.scala 81be88aa618ed5614703d45a0556b77c97290085
> core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 715845b167c44268bd7e4b76dfb69199bfb2fad4
> core/src/main/scala/kafka/server/ReplicaManager.scala 242c18d47828b7c5e6b1fc219a0f1199fb1f9512
> core/src/test/scala/unit/kafka/admin/AdminTest.scala 52d35a3ccc86a66acb57c1138d4f2fea8f4ca4b0
> core/src/test/scala/unit/kafka/log/CleanerTest.scala 5a312bf0803c1df4636e2e64ba83036a4e8e92dd
>
> Diff: https://reviews.apache.org/r/15674/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 15674: move LogCleanerManager to a separate file and
improve unit test
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15674/
-----------------------------------------------------------
(Updated Jan. 7, 2014, 6:51 p.m.)
Review request for kafka.
Summary (updated)
-----------------
move LogCleanerManager to a separate file and improve unit test
Bugs: KAFKA-1074
https://issues.apache.org/jira/browse/KAFKA-1074
Repository: kafka
Description (updated)
-------
move LogCleanerManager to a separate file and improve unit test
rename LogCleanerStates to LogCleanerManagers; optimize truncateTo to only pause cleaning if the truncated offset is not on the active segment
add test for aborting during cleaning
checkpoint recovery points in truncateFullyAndStartAt()
move cleaning states and locks to a separate class
minor fix 2
minor fix
remove unused var and exception
support pause/resume log clean and remove OptimisticLockFailureException
kafka-1074; minor fix3
kafka-1074; minor fix2
kafka-1074; minor fix
kafka-1074; better synchronization with log cleaner
kafka-1074; fix 4
kafka-1074; fix 3
kafka-1074; fix 2
kafka-1074
Diffs (updated)
-----
core/src/main/scala/kafka/cluster/Partition.scala 5c9307d71641ccc6c09a54b69d5aa2b4bc2a4cde
core/src/main/scala/kafka/common/LogCleaningAbortedException.scala PRE-CREATION
core/src/main/scala/kafka/common/OptimisticLockFailureException.scala 0e69110e71667ad75a460534f4422a7b6ec1cdc6
core/src/main/scala/kafka/log/Log.scala beda421b558544196379bd9ab7855cea7614e8e3
core/src/main/scala/kafka/log/LogCleaner.scala ccde2abd99d2204775e4d3e9836aca34eb6747a3
core/src/main/scala/kafka/log/LogCleanerManager.scala PRE-CREATION
core/src/main/scala/kafka/log/LogManager.scala 81be88aa618ed5614703d45a0556b77c97290085
core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 715845b167c44268bd7e4b76dfb69199bfb2fad4
core/src/main/scala/kafka/server/ReplicaManager.scala 242c18d47828b7c5e6b1fc219a0f1199fb1f9512
core/src/test/scala/unit/kafka/admin/AdminTest.scala 52d35a3ccc86a66acb57c1138d4f2fea8f4ca4b0
core/src/test/scala/unit/kafka/log/CleanerTest.scala 5a312bf0803c1df4636e2e64ba83036a4e8e92dd
Diff: https://reviews.apache.org/r/15674/diff/
Testing
-------
Thanks,
Jun Rao