Re: Review Request 31306: Patch for KAFKA-1755
--- 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
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
--- 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
--- 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
--- 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
--- 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
--- 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
--- 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
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
--- 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
--- 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
Re: Review Request 31306: Patch for KAFKA-1755
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
--- 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
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
--- 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
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
--- 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
--- 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
