You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Joel Koshy <jj...@gmail.com> on 2015/02/23 15:43:08 UTC
Review Request 31306: Patch for KAFKA-1755
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1755
https://issues.apache.org/jira/browse/KAFKA-1755
Repository: kafka
Description
-------
Add compacted topic constraint checks - reject unkeyed messages; reject compressed messages if topic's broker-side compression is not uncompressed
Diffs
-----
core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
Diff: https://reviews.apache.org/r/31306/diff/
Testing
-------
Thanks,
Joel Koshy
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Joel Koshy <jj...@gmail.com>.
> On Feb. 23, 2015, 6:36 p.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/message/ByteBufferMessageSet.scala, line 221
> > <https://reviews.apache.org/r/31306/diff/1/?file=872917#file872917line221>
> >
> > We may also need to check this right:
> > sourceCodec != NoCompressionCodec
Thanks for checking. I actually think this is okay since the target codec is what matters - i.e., if target==uncompressed then source codec does not matter.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review73618
-----------------------------------------------------------
On Feb. 23, 2015, 2:43 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 2:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Add compacted topic constraint checks - reject unkeyed messages; reject compressed messages if topic's broker-side compression is not uncompressed
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review73618
-----------------------------------------------------------
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala
<https://reviews.apache.org/r/31306/#comment120016>
We may also need to check this right:
sourceCodec != NoCompressionCodec
- Mayuresh Gharat
On Feb. 23, 2015, 2:43 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 2:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Add compacted topic constraint checks - reject unkeyed messages; reject compressed messages if topic's broker-side compression is not uncompressed
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Joel Koshy <jj...@gmail.com>.
> On Feb. 23, 2015, 7:05 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/message/ByteBufferMessageSet.scala, line 209
> > <https://reviews.apache.org/r/31306/diff/1/?file=872917#file872917line209>
> >
> > In doing !compactedTopic here I'm forcing iteration over the messages below. I can also do an in-place verification here to avoid iteration (and creation of message objects).
I'll upload another patch in a minute which explains this more clearly. Not sure if it is worth it - let me know what you guys think.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review73629
-----------------------------------------------------------
On Feb. 23, 2015, 2:43 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 2:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Add compacted topic constraint checks - reject unkeyed messages; reject compressed messages if topic's broker-side compression is not uncompressed
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review73629
-----------------------------------------------------------
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala
<https://reviews.apache.org/r/31306/#comment120029>
In doing !compactedTopic here I'm forcing iteration over the messages below. I can also do an in-place verification here to avoid iteration (and creation of message objects).
- Joel Koshy
On Feb. 23, 2015, 2:43 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 2:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Add compacted topic constraint checks - reject unkeyed messages; reject compressed messages if topic's broker-side compression is not uncompressed
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Joel Koshy <jj...@gmail.com>.
> On Feb. 26, 2015, 3:23 a.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/message/ByteBufferMessageSet.scala, line 205
> > <https://reviews.apache.org/r/31306/diff/2/?file=873162#file873162line205>
> >
> > Can we add the key-validation logic into analyzeAndValidateMessageSet()?
No - because we only do shallow iteration in analyzeAndValidateMessageSet - that is sort of a pre-validation step.
> On Feb. 26, 2015, 3:23 a.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/message/ByteBufferMessageSet.scala, lines 252-254
> > <https://reviews.apache.org/r/31306/diff/2/?file=873162#file873162line252>
> >
> > Why have bufferLimit if we already has sizeInBytes?
I just felt it was clearer to say bufferLimit or buffer.limt directly than sizeInBytes - I'll change it back.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review74201
-----------------------------------------------------------
On Feb. 23, 2015, 10:29 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 10:29 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1755 v2
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review74201
-----------------------------------------------------------
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala
<https://reviews.apache.org/r/31306/#comment120719>
Can we add the key-validation logic into analyzeAndValidateMessageSet()?
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala
<https://reviews.apache.org/r/31306/#comment120718>
Why have bufferLimit if we already has sizeInBytes?
- Guozhang Wang
On Feb. 23, 2015, 10:29 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 10:29 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1755 v2
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review73692
-----------------------------------------------------------
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala
<https://reviews.apache.org/r/31306/#comment120077>
(I added this because we were using the sizeInBytes just to check the buffer limit above and it looked confusing)
core/src/test/scala/unit/kafka/log/LogTest.scala
<https://reviews.apache.org/r/31306/#comment120076>
(unintentional)
- Joel Koshy
On Feb. 23, 2015, 10:29 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 10:29 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1755 v2
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review74329
-----------------------------------------------------------
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala
<https://reviews.apache.org/r/31306/#comment120894>
Also, when reviewing it would help if the reviewer compares this portion of the code to the initial patch (v1)
In v1 I don't do in-place validation. i.e., I always do a deep iteration in the branch below. This is more efficient but also slightly more complicated.
- Joel Koshy
On Feb. 26, 2015, 6:54 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1755 v2
>
>
> re-enable test
>
>
> Incorporate Guozhang's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review74992
-----------------------------------------------------------
Ship it!
Ship It!
- Mayuresh Gharat
On Feb. 26, 2015, 6:54 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1755 v2
>
>
> re-enable test
>
>
> Incorporate Guozhang's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review75112
-----------------------------------------------------------
core/src/test/scala/unit/kafka/log/CleanerTest.scala
<https://reviews.apache.org/r/31306/#comment122056>
fixed
- Joel Koshy
On Feb. 26, 2015, 6:54 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1755 v2
>
>
> re-enable test
>
>
> Incorporate Guozhang's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review74985
-----------------------------------------------------------
Ship it!
Ship It!
- Guozhang Wang
On Feb. 26, 2015, 6:54 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1755 v2
>
>
> re-enable test
>
>
> Incorporate Guozhang's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Joel Koshy <jj...@gmail.com>.
> On March 3, 2015, 5:43 p.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/log/LogCleaner.scala, line 413
> > <https://reviews.apache.org/r/31306/diff/3/?file=878588#file878588line413>
> >
> > This will mean that if there are unkeyed messages we will neglect them and not throw an exception right?
> > So we are aloowing unkeyed messages to go through comapction.
Unkeyed messages will be rejected up front (as demonstrated in the unit tests). However, if you change a topic to be compacted (from non-compacted) and it already has unkeyed messages then those will just be ignored (i.e., deleted).
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review74986
-----------------------------------------------------------
On Feb. 26, 2015, 6:54 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1755 v2
>
>
> re-enable test
>
>
> Incorporate Guozhang's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review74986
-----------------------------------------------------------
core/src/main/scala/kafka/log/LogCleaner.scala
<https://reviews.apache.org/r/31306/#comment121866>
This will mean that if there are unkeyed messages we will neglect them and not throw an exception right?
So we are aloowing unkeyed messages to go through comapction.
- Mayuresh Gharat
On Feb. 26, 2015, 6:54 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1755 v2
>
>
> re-enable test
>
>
> Incorporate Guozhang's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review75047
-----------------------------------------------------------
core/src/test/scala/unit/kafka/log/CleanerTest.scala
<https://reviews.apache.org/r/31306/#comment121983>
The comment is incorrect. We are appending keyed messages here.
- Jun Rao
On Feb. 26, 2015, 6:54 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2015, 6:54 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1755 v2
>
>
> re-enable test
>
>
> Incorporate Guozhang's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/
-----------------------------------------------------------
(Updated Feb. 26, 2015, 6:54 p.m.)
Review request for kafka.
Bugs: KAFKA-1755
https://issues.apache.org/jira/browse/KAFKA-1755
Repository: kafka
Description (updated)
-------
KAFKA-1755 v2
re-enable test
Incorporate Guozhang's comments
Diffs (updated)
-----
core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
Diff: https://reviews.apache.org/r/31306/diff/
Testing
-------
Thanks,
Joel Koshy
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/
-----------------------------------------------------------
(Updated Feb. 23, 2015, 10:29 p.m.)
Review request for kafka.
Bugs: KAFKA-1755
https://issues.apache.org/jira/browse/KAFKA-1755
Repository: kafka
Description (updated)
-------
KAFKA-1755 v2
Diffs (updated)
-----
core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
Diff: https://reviews.apache.org/r/31306/diff/
Testing
-------
Thanks,
Joel Koshy
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Joel Koshy <jj...@gmail.com>.
> On Feb. 23, 2015, 5:08 p.m., Neha Narkhede wrote:
> > It also makes a lot of sense to disallow setting a compacted topic to uncompacted and vice versa without deleting the topic. Was there a reason to not include that change here or are you planning on including it in your follow-up patch?
Thanks for the quick review. Re: preventing compact<->non-compact retention changes: I think it is better to do that as a separate patch or a separate jira altogether.
> On Feb. 23, 2015, 5:08 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/log/LogCleaner.scala, line 257
> > <https://reviews.apache.org/r/31306/diff/1/?file=872915#file872915line257>
> >
> > Is it necessary to log this in WARN? It seems like if you hit this issue on the broker, you will know through the jmx value anyway and the WARN message will just keep polluting the logs till the issue is fixed. Maybe turn it down to DEBUG?
I think it is better to keep this at warn or even make it error - for better visibility for operations. This only warns if there are invalid messages and it only logs one line at the end of the cleaner line.
(BTW, which jmx value are you referring to?)
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review73584
-----------------------------------------------------------
On Feb. 23, 2015, 2:43 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 2:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Add compacted topic constraint checks - reject unkeyed messages; reject compressed messages if topic's broker-side compression is not uncompressed
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 31306: Patch for KAFKA-1755
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31306/#review73584
-----------------------------------------------------------
It also makes a lot of sense to disallow setting a compacted topic to uncompacted and vice versa without deleting the topic. Was there a reason to not include that change here or are you planning on including it in your follow-up patch?
core/src/main/scala/kafka/log/LogCleaner.scala
<https://reviews.apache.org/r/31306/#comment119979>
Is it necessary to log this in WARN? It seems like if you hit this issue on the broker, you will know through the jmx value anyway and the WARN message will just keep polluting the logs till the issue is fixed. Maybe turn it down to DEBUG?
- Neha Narkhede
On Feb. 23, 2015, 2:43 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31306/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 2:43 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1755
> https://issues.apache.org/jira/browse/KAFKA-1755
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Add compacted topic constraint checks - reject unkeyed messages; reject compressed messages if topic's broker-side compression is not uncompressed
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/log/Log.scala 846023bb98d0fa0603016466360c97071ac935ea
> core/src/main/scala/kafka/log/LogCleaner.scala f8e7cd5fabce78c248a9027c4bb374a792508675
> core/src/main/scala/kafka/log/LogCleanerManager.scala fd87d90597981c867a9b23731fca3b555bf85b7f
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala f46ad5cbbbad77d8d1f490d1f8aac97858da9b06
> core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb
> core/src/test/scala/unit/kafka/log/CleanerTest.scala d10e4f4ccbca5e50d81a243d3ab30cc7314b7fef
> core/src/test/scala/unit/kafka/log/LogTest.scala c2dd8eb69da8c0982a0dd20231c6f8bd58eb623e
> core/src/test/scala/unit/kafka/message/ByteBufferMessageSetTest.scala 73a26377eb63ab9989698e0491049434f032cba2
>
> Diff: https://reviews.apache.org/r/31306/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joel Koshy
>
>