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 2015/01/16 01:52:37 UTC
Review Request 29952: Patch for kafka-1864
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29952/
-----------------------------------------------------------
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
Posted by Jun Rao <ju...@gmail.com>.
> 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
Posted by Gwen Shapira <gs...@cloudera.com>.
> 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
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
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
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jun Rao <ju...@gmail.com>.
> 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
>
>
Re: Review Request 29952: Patch for kafka-1864
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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
>
>