Re: Review Request 29952: Patch for kafka-1864
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/#review68437 --- Ship it! Ship It! - Neha Narkhede On Jan. 16, 2015, 12:52 a.m., Jun Rao wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/ --- (Updated Jan. 16, 2015, 12:52 a.m.) Review request for kafka. Bugs: kafka-1864 https://issues.apache.org/jira/browse/kafka-1864 Repository: kafka Description --- create offset topic with a larger replication factor by default Diffs - core/src/main/scala/kafka/server/KafkaApis.scala d626b1710813648524eefa5a3df098393c3e7743 core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f core/src/main/scala/kafka/server/OffsetManager.scala 43eb2a35bb54d32c66cdb94772df657b3a104d1a core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 07a7beee9dec733eae943b425ae58c54f08458d8 core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 4a3a5b264a021e55c39f4d7424ce04ee591503ef Diff: https://reviews.apache.org/r/29952/diff/ Testing --- Thanks, Jun Rao
Re: Review Request 29952: Patch for kafka-1864
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/#review68395 --- core/src/main/scala/kafka/server/OffsetManager.scala https://reviews.apache.org/r/29952/#comment112588 I'm wondering why you chose to change defaults here and not in KafkaConfig? Unless I'm missing something, it looks like we are defining defaults in two different places, and they don't match any more. - Gwen Shapira On Jan. 16, 2015, 12:52 a.m., Jun Rao wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/ --- (Updated Jan. 16, 2015, 12:52 a.m.) Review request for kafka. Bugs: kafka-1864 https://issues.apache.org/jira/browse/kafka-1864 Repository: kafka Description --- create offset topic with a larger replication factor by default Diffs - core/src/main/scala/kafka/server/KafkaApis.scala d626b1710813648524eefa5a3df098393c3e7743 core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f core/src/main/scala/kafka/server/OffsetManager.scala 43eb2a35bb54d32c66cdb94772df657b3a104d1a core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 07a7beee9dec733eae943b425ae58c54f08458d8 core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 4a3a5b264a021e55c39f4d7424ce04ee591503ef Diff: https://reviews.apache.org/r/29952/diff/ Testing --- Thanks, Jun Rao
Re: Review Request 29952: Patch for kafka-1864
On Jan. 16, 2015, 2:54 a.m., Gwen Shapira wrote: core/src/main/scala/kafka/server/OffsetManager.scala, line 77 https://reviews.apache.org/r/29952/diff/1/?file=823279#file823279line77 I'm wondering why you chose to change defaults here and not in KafkaConfig? Unless I'm missing something, it looks like we are defining defaults in two different places, and they don't match any more. KafkaConfig references this value in OffsetManager, right? - Jun --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/#review68395 --- On Jan. 16, 2015, 12:52 a.m., Jun Rao wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/ --- (Updated Jan. 16, 2015, 12:52 a.m.) Review request for kafka. Bugs: kafka-1864 https://issues.apache.org/jira/browse/kafka-1864 Repository: kafka Description --- create offset topic with a larger replication factor by default Diffs - core/src/main/scala/kafka/server/KafkaApis.scala d626b1710813648524eefa5a3df098393c3e7743 core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f core/src/main/scala/kafka/server/OffsetManager.scala 43eb2a35bb54d32c66cdb94772df657b3a104d1a core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 07a7beee9dec733eae943b425ae58c54f08458d8 core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 4a3a5b264a021e55c39f4d7424ce04ee591503ef Diff: https://reviews.apache.org/r/29952/diff/ Testing --- Thanks, Jun Rao
Re: Review Request 29952: Patch for kafka-1864
On Jan. 16, 2015, 2:54 a.m., Gwen Shapira wrote: core/src/main/scala/kafka/server/OffsetManager.scala, line 77 https://reviews.apache.org/r/29952/diff/1/?file=823279#file823279line77 I'm wondering why you chose to change defaults here and not in KafkaConfig? Unless I'm missing something, it looks like we are defining defaults in two different places, and they don't match any more. Jun Rao wrote: KafkaConfig references this value in OffsetManager, right? Right, sorry - got confused. Typically its the other way around, I think. But this will work too. - Gwen --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/#review68395 --- On Jan. 16, 2015, 12:52 a.m., Jun Rao wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/ --- (Updated Jan. 16, 2015, 12:52 a.m.) Review request for kafka. Bugs: kafka-1864 https://issues.apache.org/jira/browse/kafka-1864 Repository: kafka Description --- create offset topic with a larger replication factor by default Diffs - core/src/main/scala/kafka/server/KafkaApis.scala d626b1710813648524eefa5a3df098393c3e7743 core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f core/src/main/scala/kafka/server/OffsetManager.scala 43eb2a35bb54d32c66cdb94772df657b3a104d1a core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 07a7beee9dec733eae943b425ae58c54f08458d8 core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 4a3a5b264a021e55c39f4d7424ce04ee591503ef Diff: https://reviews.apache.org/r/29952/diff/ Testing --- Thanks, Jun Rao
Re: Review Request 29952: Patch for kafka-1864
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/#review68381 --- core/src/main/scala/kafka/server/OffsetManager.scala https://reviews.apache.org/r/29952/#comment112577 The only issue here is the problem raised in KAFKA-1867 - even though that should not happen in practice since you would generally only commit offsets after topics do exist in the cluster. Anyway, wouldn't it just be simpler to keep the replication factor default as 1 given that it is possible to change it? - Joel Koshy On Jan. 16, 2015, 12:52 a.m., Jun Rao wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/ --- (Updated Jan. 16, 2015, 12:52 a.m.) Review request for kafka. Bugs: kafka-1864 https://issues.apache.org/jira/browse/kafka-1864 Repository: kafka Description --- create offset topic with a larger replication factor by default Diffs - core/src/main/scala/kafka/server/KafkaApis.scala d626b1710813648524eefa5a3df098393c3e7743 core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f core/src/main/scala/kafka/server/OffsetManager.scala 43eb2a35bb54d32c66cdb94772df657b3a104d1a core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 07a7beee9dec733eae943b425ae58c54f08458d8 core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 4a3a5b264a021e55c39f4d7424ce04ee591503ef Diff: https://reviews.apache.org/r/29952/diff/ Testing --- Thanks, Jun Rao
Re: Review Request 29952: Patch for kafka-1864
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/#review68406 --- Ship it! core/src/main/scala/kafka/server/OffsetManager.scala https://reviews.apache.org/r/29952/#comment112600 Although I think the documentation makes this clear the default should probably be conservative so this sounds reasonable. - Joel Koshy On Jan. 16, 2015, 12:52 a.m., Jun Rao wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/ --- (Updated Jan. 16, 2015, 12:52 a.m.) Review request for kafka. Bugs: kafka-1864 https://issues.apache.org/jira/browse/kafka-1864 Repository: kafka Description --- create offset topic with a larger replication factor by default Diffs - core/src/main/scala/kafka/server/KafkaApis.scala d626b1710813648524eefa5a3df098393c3e7743 core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f core/src/main/scala/kafka/server/OffsetManager.scala 43eb2a35bb54d32c66cdb94772df657b3a104d1a core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 07a7beee9dec733eae943b425ae58c54f08458d8 core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 4a3a5b264a021e55c39f4d7424ce04ee591503ef Diff: https://reviews.apache.org/r/29952/diff/ Testing --- Thanks, Jun Rao
Re: Review Request 29952: Patch for kafka-1864
On Jan. 16, 2015, 1:25 a.m., Joel Koshy wrote: core/src/main/scala/kafka/server/OffsetManager.scala, line 79 https://reviews.apache.org/r/29952/diff/1/?file=823279#file823279line79 The only issue here is the problem raised in KAFKA-1867 - even though that should not happen in practice since you would generally only commit offsets after topics do exist in the cluster. Anyway, wouldn't it just be simpler to keep the replication factor default as 1 given that it is possible to change it? The main purpose of the patch is to make the default behavior good. For that, we want to have enough partitions and enough redundancy. The issue with defaulting replication factor to 1 is that people may not realize the issue until one of the brokers goes down. At that time, it's too late to change the replication factor. - Jun --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/#review68381 --- On Jan. 16, 2015, 12:52 a.m., Jun Rao wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29952/ --- (Updated Jan. 16, 2015, 12:52 a.m.) Review request for kafka. Bugs: kafka-1864 https://issues.apache.org/jira/browse/kafka-1864 Repository: kafka Description --- create offset topic with a larger replication factor by default Diffs - core/src/main/scala/kafka/server/KafkaApis.scala d626b1710813648524eefa5a3df098393c3e7743 core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f core/src/main/scala/kafka/server/OffsetManager.scala 43eb2a35bb54d32c66cdb94772df657b3a104d1a core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 07a7beee9dec733eae943b425ae58c54f08458d8 core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 4a3a5b264a021e55c39f4d7424ce04ee591503ef Diff: https://reviews.apache.org/r/29952/diff/ Testing --- Thanks, Jun Rao
