You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Grant Henke <gr...@gmail.com> on 2015/07/16 16:33:01 UTC
Review Request 36548: Patch for KAFKA-2336
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-2336
https://issues.apache.org/jira/browse/KAFKA-2336
Repository: kafka
Description
-------
KAFKA-2336: Changing offsets.topic.num.partitions after the offset topic is created breaks consumer group partition assignment
Diffs
-----
core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
Diff: https://reviews.apache.org/r/36548/diff/
Testing
-------
Thanks,
Grant Henke
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Grant Henke <gr...@gmail.com>.
> On July 16, 2015, 4:53 p.m., Edward Ribeiro wrote:
> > core/src/main/scala/kafka/server/OffsetManager.scala, line 454
> > <https://reviews.apache.org/r/36548/diff/1/?file=1013403#file1013403line454>
> >
> > It looks like there's a mix of "if(" and "if (", with the latter being more used than the former. For this reason, I think it would be nice to put a space between "if" and parenthesis, and maybe we should open a ticket to define and uniform the coding style of the project (if one doesn't exists already).
>
> Grant Henke wrote:
> There is a coding guide here: http://kafka.apache.org/coding-guide.html
>
> Spaces after if statements is not mentioned. I don't have strong feeling either way.
I will drop this issue for this specific patch. A standard can definitely be addressed and made uniform in a new Jira.
- Grant
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review91898
-----------------------------------------------------------
On July 16, 2015, 2:33 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 2:33 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2336: Changing offsets.topic.num.partitions after the offset topic is created breaks consumer group partition assignment
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Grant Henke <gr...@gmail.com>.
> On July 16, 2015, 4:53 p.m., Edward Ribeiro wrote:
> > core/src/main/scala/kafka/server/OffsetManager.scala, line 454
> > <https://reviews.apache.org/r/36548/diff/1/?file=1013403#file1013403line454>
> >
> > It looks like there's a mix of "if(" and "if (", with the latter being more used than the former. For this reason, I think it would be nice to put a space between "if" and parenthesis, and maybe we should open a ticket to define and uniform the coding style of the project (if one doesn't exists already).
There is a coding guide here: http://kafka.apache.org/coding-guide.html
Spaces after if statements is not mentioned. I don't have strong feeling either way.
- Grant
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review91898
-----------------------------------------------------------
On July 16, 2015, 2:33 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 2:33 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2336: Changing offsets.topic.num.partitions after the offset topic is created breaks consumer group partition assignment
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Grant Henke <gr...@gmail.com>.
> On July 16, 2015, 4:53 p.m., Edward Ribeiro wrote:
> > core/src/main/scala/kafka/server/OffsetManager.scala, line 454
> > <https://reviews.apache.org/r/36548/diff/1/?file=1013403#file1013403line454>
> >
> > It looks like there's a mix of "if(" and "if (", with the latter being more used than the former. For this reason, I think it would be nice to put a space between "if" and parenthesis, and maybe we should open a ticket to define and uniform the coding style of the project (if one doesn't exists already).
>
> Grant Henke wrote:
> There is a coding guide here: http://kafka.apache.org/coding-guide.html
>
> Spaces after if statements is not mentioned. I don't have strong feeling either way.
>
> Grant Henke wrote:
> I will drop this issue for this specific patch. A standard can definitely be addressed and made uniform in a new Jira.
>
> Ismael Juma wrote:
> The Scala standard is to add a space after the `if`.
That's true: http://docs.scala-lang.org/style/control-structures.html#trivial_conditionals
I will update the patch.
Standardizing the existing code (if thought to be important) is something that should then be done in a new patch or jira. A tool like https://github.com/mdr/scalariform may be worth considering too.
- Grant
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review91898
-----------------------------------------------------------
On July 16, 2015, 2:33 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 2:33 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2336: Changing offsets.topic.num.partitions after the offset topic is created breaks consumer group partition assignment
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Ismael Juma <mi...@juma.me.uk>.
> On July 16, 2015, 4:53 p.m., Edward Ribeiro wrote:
> > core/src/main/scala/kafka/server/OffsetManager.scala, line 454
> > <https://reviews.apache.org/r/36548/diff/1/?file=1013403#file1013403line454>
> >
> > It looks like there's a mix of "if(" and "if (", with the latter being more used than the former. For this reason, I think it would be nice to put a space between "if" and parenthesis, and maybe we should open a ticket to define and uniform the coding style of the project (if one doesn't exists already).
>
> Grant Henke wrote:
> There is a coding guide here: http://kafka.apache.org/coding-guide.html
>
> Spaces after if statements is not mentioned. I don't have strong feeling either way.
>
> Grant Henke wrote:
> I will drop this issue for this specific patch. A standard can definitely be addressed and made uniform in a new Jira.
>
> Ismael Juma wrote:
> The Scala standard is to add a space after the `if`.
>
> Grant Henke wrote:
> That's true: http://docs.scala-lang.org/style/control-structures.html#trivial_conditionals
> I will update the patch.
>
> Standardizing the existing code (if thought to be important) is something that should then be done in a new patch or jira. A tool like https://github.com/mdr/scalariform may be worth considering too.
Yes, Grant, agreed.
- Ismael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review91898
-----------------------------------------------------------
On July 16, 2015, 2:33 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 2:33 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2336: Changing offsets.topic.num.partitions after the offset topic is created breaks consumer group partition assignment
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Ismael Juma <mi...@juma.me.uk>.
> On July 16, 2015, 4:53 p.m., Edward Ribeiro wrote:
> > core/src/main/scala/kafka/server/OffsetManager.scala, line 454
> > <https://reviews.apache.org/r/36548/diff/1/?file=1013403#file1013403line454>
> >
> > It looks like there's a mix of "if(" and "if (", with the latter being more used than the former. For this reason, I think it would be nice to put a space between "if" and parenthesis, and maybe we should open a ticket to define and uniform the coding style of the project (if one doesn't exists already).
>
> Grant Henke wrote:
> There is a coding guide here: http://kafka.apache.org/coding-guide.html
>
> Spaces after if statements is not mentioned. I don't have strong feeling either way.
>
> Grant Henke wrote:
> I will drop this issue for this specific patch. A standard can definitely be addressed and made uniform in a new Jira.
The Scala standard is to add a space after the `if`.
- Ismael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review91898
-----------------------------------------------------------
On July 16, 2015, 2:33 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 2:33 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2336: Changing offsets.topic.num.partitions after the offset topic is created breaks consumer group partition assignment
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Edward Ribeiro <ed...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review91898
-----------------------------------------------------------
core/src/main/scala/kafka/server/OffsetManager.scala (line 453)
<https://reviews.apache.org/r/36548/#comment145611>
It looks like there's a mix of "if(" and "if (", with the latter being more used than the former. For this reason, I think it would be nice to put a space between "if" and parenthesis, and maybe we should open a ticket to define and uniform the coding style of the project (if one doesn't exists already).
- Edward Ribeiro
On July 16, 2015, 2:33 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 2:33 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2336: Changing offsets.topic.num.partitions after the offset topic is created breaks consumer group partition assignment
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Gwen Shapira <gs...@cloudera.com>.
> On July 17, 2015, 4:26 a.m., Jiangjie Qin wrote:
> > Looks good to me.
>
> Jiangjie Qin wrote:
> Actually do we need to talk to Zookeeper every time? Can we read the data from topic metadata cache directly?
Good point, Jiangjie - looks like partitionFor is called on every ConsumerMetadataRequest handling, so some kind of caching will be nice.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review92020
-----------------------------------------------------------
On July 16, 2015, 6:04 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 6:04 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix Scala style
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Jiangjie Qin <be...@gmail.com>.
> On July 17, 2015, 4:26 a.m., Jiangjie Qin wrote:
> > Looks good to me.
Actually do we need to talk to Zookeeper every time? Can we read the data from topic metadata cache directly?
- Jiangjie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review92020
-----------------------------------------------------------
On July 16, 2015, 6:04 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 6:04 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix Scala style
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Jiangjie Qin <be...@gmail.com>.
> On July 17, 2015, 4:26 a.m., Jiangjie Qin wrote:
> > Looks good to me.
>
> Jiangjie Qin wrote:
> Actually do we need to talk to Zookeeper every time? Can we read the data from topic metadata cache directly?
>
> Gwen Shapira wrote:
> Good point, Jiangjie - looks like partitionFor is called on every ConsumerMetadataRequest handling, so some kind of caching will be nice.
>
> Grant Henke wrote:
> We only talk to Zookeeper once at instance creation via the call to `getOffsetsTopicPartitionCount` and setting the val `offsetsTopicPartitionCount`. The static value is used from then on for every call to `partitionFor`.
Ah, I see, my bad. Then this patch seems not completely solve the issue, though. Let's say offsets topic is not exist yet. What if two brokers had different offset topic partition number configuration? After they startup and before the offset topic get created in zookeeper, they will have different value for offsetsTopicPartitionCount. Will that cause problem silently?
- Jiangjie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review92020
-----------------------------------------------------------
On July 16, 2015, 6:04 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 6:04 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix Scala style
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Grant Henke <gr...@gmail.com>.
> On July 17, 2015, 4:26 a.m., Jiangjie Qin wrote:
> > Looks good to me.
>
> Jiangjie Qin wrote:
> Actually do we need to talk to Zookeeper every time? Can we read the data from topic metadata cache directly?
>
> Gwen Shapira wrote:
> Good point, Jiangjie - looks like partitionFor is called on every ConsumerMetadataRequest handling, so some kind of caching will be nice.
>
> Grant Henke wrote:
> We only talk to Zookeeper once at instance creation via the call to `getOffsetsTopicPartitionCount` and setting the val `offsetsTopicPartitionCount`. The static value is used from then on for every call to `partitionFor`.
>
> Jiangjie Qin wrote:
> Ah, I see, my bad. Then this patch seems not completely solve the issue, though. Let's say offsets topic is not exist yet. What if two brokers had different offset topic partition number configuration? After they startup and before the offset topic get created in zookeeper, they will have different value for offsetsTopicPartitionCount. Will that cause problem silently?
I think that sort of issue exists for many of Kafka's configurations, and exists for this configuration without this patch too. I do not aim to solve non-uniform configuration in this patch.
- Grant
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review92020
-----------------------------------------------------------
On July 16, 2015, 6:04 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 6:04 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix Scala style
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Grant Henke <gr...@gmail.com>.
> On July 17, 2015, 4:26 a.m., Jiangjie Qin wrote:
> > Looks good to me.
>
> Jiangjie Qin wrote:
> Actually do we need to talk to Zookeeper every time? Can we read the data from topic metadata cache directly?
>
> Gwen Shapira wrote:
> Good point, Jiangjie - looks like partitionFor is called on every ConsumerMetadataRequest handling, so some kind of caching will be nice.
We only talk to Zookeeper once at instance creation via the call to `getOffsetsTopicPartitionCount` and setting the val `offsetsTopicPartitionCount`. The static value is used from then on for every call to `partitionFor`.
- Grant
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review92020
-----------------------------------------------------------
On July 16, 2015, 6:04 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 6:04 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix Scala style
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Grant Henke <gr...@gmail.com>.
> On July 17, 2015, 4:26 a.m., Jiangjie Qin wrote:
> > Looks good to me.
>
> Jiangjie Qin wrote:
> Actually do we need to talk to Zookeeper every time? Can we read the data from topic metadata cache directly?
>
> Gwen Shapira wrote:
> Good point, Jiangjie - looks like partitionFor is called on every ConsumerMetadataRequest handling, so some kind of caching will be nice.
>
> Grant Henke wrote:
> We only talk to Zookeeper once at instance creation via the call to `getOffsetsTopicPartitionCount` and setting the val `offsetsTopicPartitionCount`. The static value is used from then on for every call to `partitionFor`.
>
> Jiangjie Qin wrote:
> Ah, I see, my bad. Then this patch seems not completely solve the issue, though. Let's say offsets topic is not exist yet. What if two brokers had different offset topic partition number configuration? After they startup and before the offset topic get created in zookeeper, they will have different value for offsetsTopicPartitionCount. Will that cause problem silently?
>
> Grant Henke wrote:
> I think that sort of issue exists for many of Kafka's configurations, and exists for this configuration without this patch too. I do not aim to solve non-uniform configuration in this patch.
>
> Jiangjie Qin wrote:
> I agree that configuration mismatch could cause issue for other configurations as well. But having existing problems does not mean we should introduce one more to them. Besides, is it a simpler solution to just read from topic metadata cache?
>
> Gwen Shapira wrote:
> Was there a resolution here?
>
> Grant, will reading number of partitions from topic metadata cache solve the edge case that Becket raised?
This is something that I have been thinking about for a little while. I think reading from topic metadata would help the scenario Becket raised, though it is a very rare edge case and would only occur when both the offset topic does not exist yet and the brokers do not have uniform configurations.
I can make the change to use the cache, but the thing is I will either need to pass it via the constructors down through ConsumerCoordinator and OffsetManager from KafkaApis or through the partitionsFor calls in each. And though its sounds mroe simple to use a cache...chaches are never simple. These are the trade offs I have been considering.
- Grant
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review92020
-----------------------------------------------------------
On July 16, 2015, 6:04 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 6:04 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix Scala style
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Jiangjie Qin <be...@gmail.com>.
> On July 17, 2015, 4:26 a.m., Jiangjie Qin wrote:
> > Looks good to me.
>
> Jiangjie Qin wrote:
> Actually do we need to talk to Zookeeper every time? Can we read the data from topic metadata cache directly?
>
> Gwen Shapira wrote:
> Good point, Jiangjie - looks like partitionFor is called on every ConsumerMetadataRequest handling, so some kind of caching will be nice.
>
> Grant Henke wrote:
> We only talk to Zookeeper once at instance creation via the call to `getOffsetsTopicPartitionCount` and setting the val `offsetsTopicPartitionCount`. The static value is used from then on for every call to `partitionFor`.
>
> Jiangjie Qin wrote:
> Ah, I see, my bad. Then this patch seems not completely solve the issue, though. Let's say offsets topic is not exist yet. What if two brokers had different offset topic partition number configuration? After they startup and before the offset topic get created in zookeeper, they will have different value for offsetsTopicPartitionCount. Will that cause problem silently?
>
> Grant Henke wrote:
> I think that sort of issue exists for many of Kafka's configurations, and exists for this configuration without this patch too. I do not aim to solve non-uniform configuration in this patch.
I agree that configuration mismatch could cause issue for other configurations as well. But having existing problems does not mean we should introduce one more to them. Besides, is it a simpler solution to just read from topic metadata cache?
- Jiangjie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review92020
-----------------------------------------------------------
On July 16, 2015, 6:04 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 6:04 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix Scala style
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Gwen Shapira <gs...@cloudera.com>.
> On July 17, 2015, 4:26 a.m., Jiangjie Qin wrote:
> > Looks good to me.
>
> Jiangjie Qin wrote:
> Actually do we need to talk to Zookeeper every time? Can we read the data from topic metadata cache directly?
>
> Gwen Shapira wrote:
> Good point, Jiangjie - looks like partitionFor is called on every ConsumerMetadataRequest handling, so some kind of caching will be nice.
>
> Grant Henke wrote:
> We only talk to Zookeeper once at instance creation via the call to `getOffsetsTopicPartitionCount` and setting the val `offsetsTopicPartitionCount`. The static value is used from then on for every call to `partitionFor`.
>
> Jiangjie Qin wrote:
> Ah, I see, my bad. Then this patch seems not completely solve the issue, though. Let's say offsets topic is not exist yet. What if two brokers had different offset topic partition number configuration? After they startup and before the offset topic get created in zookeeper, they will have different value for offsetsTopicPartitionCount. Will that cause problem silently?
>
> Grant Henke wrote:
> I think that sort of issue exists for many of Kafka's configurations, and exists for this configuration without this patch too. I do not aim to solve non-uniform configuration in this patch.
>
> Jiangjie Qin wrote:
> I agree that configuration mismatch could cause issue for other configurations as well. But having existing problems does not mean we should introduce one more to them. Besides, is it a simpler solution to just read from topic metadata cache?
Was there a resolution here?
Grant, will reading number of partitions from topic metadata cache solve the edge case that Becket raised?
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review92020
-----------------------------------------------------------
On July 16, 2015, 6:04 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 6:04 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix Scala style
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review92020
-----------------------------------------------------------
Ship it!
Looks good to me.
- Jiangjie Qin
On July 16, 2015, 6:04 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 6:04 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix Scala style
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Jiangjie Qin <be...@gmail.com>.
> On Aug. 11, 2015, 10:08 p.m., Gwen Shapira wrote:
> > Ship It!
>
> Gwen Shapira wrote:
> Jiangjie, I commited despite your concerns since this patch fixes a huge potential issue.
>
> If you have an idea for an improved fix, we can tackle this in a follow up.
Thanks Gwen. I am fine with the current patch considering people are unlikely to have config discrepancies.
- Jiangjie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review95012
-----------------------------------------------------------
On Aug. 11, 2015, 3:37 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2015, 3:37 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2336: Changing offsets.topic.num.partitions after the offset topic is created breaks consumer group partition assignment
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Gwen Shapira <gs...@cloudera.com>.
> On Aug. 11, 2015, 10:08 p.m., Gwen Shapira wrote:
> > Ship It!
Jiangjie, I commited despite your concerns since this patch fixes a huge potential issue.
If you have an idea for an improved fix, we can tackle this in a follow up.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review95012
-----------------------------------------------------------
On Aug. 11, 2015, 3:37 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2015, 3:37 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2336: Changing offsets.topic.num.partitions after the offset topic is created breaks consumer group partition assignment
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review95012
-----------------------------------------------------------
Ship it!
Ship It!
- Gwen Shapira
On Aug. 11, 2015, 3:37 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2015, 3:37 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2336: Changing offsets.topic.num.partitions after the offset topic is created breaks consumer group partition assignment
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Grant Henke <gr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/
-----------------------------------------------------------
(Updated Aug. 11, 2015, 3:37 p.m.)
Review request for kafka.
Bugs: KAFKA-2336
https://issues.apache.org/jira/browse/KAFKA-2336
Repository: kafka
Description (updated)
-------
KAFKA-2336: Changing offsets.topic.num.partitions after the offset topic is created breaks consumer group partition assignment
Diffs (updated)
-----
core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
Diff: https://reviews.apache.org/r/36548/diff/
Testing
-------
Thanks,
Grant Henke
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Grant Henke <gr...@gmail.com>.
> On July 21, 2015, 2:43 p.m., Ismael Juma wrote:
> > core/src/main/scala/kafka/server/OffsetManager.scala, line 454
> > <https://reviews.apache.org/r/36548/diff/2/?file=1013441#file1013441line454>
> >
> > Is `topicData` guaranteed to have a key for `topic`? If not, it's better to do `topicData.get(topic)` to avoid the `NoSuchElementException`.
Yes it is guranteed. See ZkUtils.getPartitionAssignmentForTopics.
- Grant
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review92406
-----------------------------------------------------------
On July 16, 2015, 6:04 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 6:04 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix Scala style
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Ismael Juma <mi...@juma.me.uk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/#review92406
-----------------------------------------------------------
core/src/main/scala/kafka/server/OffsetManager.scala (line 447)
<https://reviews.apache.org/r/36548/#comment146576>
Typo, I mean you mean `of` instead of `off`.
core/src/main/scala/kafka/server/OffsetManager.scala (line 453)
<https://reviews.apache.org/r/36548/#comment146577>
Is `topicData` guaranteed to have a key for `topic`? If not, it's better to do `topicData.get(topic)` to avoid the `NoSuchElementException`.
- Ismael Juma
On July 16, 2015, 6:04 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36548/
> -----------------------------------------------------------
>
> (Updated July 16, 2015, 6:04 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2336
> https://issues.apache.org/jira/browse/KAFKA-2336
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix Scala style
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
>
> Diff: https://reviews.apache.org/r/36548/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 36548: Patch for KAFKA-2336
Posted by Grant Henke <gr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36548/
-----------------------------------------------------------
(Updated July 16, 2015, 6:04 p.m.)
Review request for kafka.
Bugs: KAFKA-2336
https://issues.apache.org/jira/browse/KAFKA-2336
Repository: kafka
Description (updated)
-------
Fix Scala style
Diffs (updated)
-----
core/src/main/scala/kafka/server/OffsetManager.scala 47b6ce93da320a565435b4a7916a0c4371143b8a
Diff: https://reviews.apache.org/r/36548/diff/
Testing
-------
Thanks,
Grant Henke