You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jay Kreps <bo...@gmail.com> on 2014/04/18 02:36:19 UTC
Review Request 20471: Patch for KAFKA-1398
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1398
https://issues.apache.org/jira/browse/KAFKA-1398
Repository: kafka
Description
-------
KAFKA-1398 dynamic config changes are broken.
Diffs
-----
core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/20471/diff/
Testing
-------
Thanks,
Jay Kreps
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Jay Kreps <bo...@gmail.com>.
> On April 18, 2014, 1:17 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/TopicConfigManager.scala, line 62
> > <https://reviews.apache.org/r/20471/diff/1/?file=561891#file561891line62>
> >
> > Since this is not currently exposed as a config should we just remove the parameter and inline it?
I actually think it is helpful to call it out as a external value. I don't think we need to make it configurable, but maybe someday?
> On April 18, 2014, 1:17 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/TopicConfigManager.scala, line 89
> > <https://reviews.apache.org/r/20471/diff/1/?file=561891#file561891line89>
> >
> > I think you intended to remove the .format (after removing the %d format specifier); or maybe log the entire change set? I think logging the list of changes would be useful.
Ack, nice catch! Thanks!
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40726
-----------------------------------------------------------
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40726
-----------------------------------------------------------
Ship it!
Looks good except for the minor issues below.
core/src/main/scala/kafka/server/TopicConfigManager.scala
<https://reviews.apache.org/r/20471/#comment73866>
Since this is not currently exposed as a config should we just remove the parameter and inline it?
core/src/main/scala/kafka/server/TopicConfigManager.scala
<https://reviews.apache.org/r/20471/#comment73869>
I think you intended to remove the .format (after removing the %d format specifier); or maybe log the entire change set? I think logging the list of changes would be useful.
- Joel Koshy
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Jay Kreps <bo...@gmail.com>.
> On April 18, 2014, 3:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/TopicConfigManager.scala, lines 87-88
> > <https://reviews.apache.org/r/20471/diff/1/?file=561891#file561891line87>
> >
> > The child list returned by ZK doesn't guarantee any ordering. We will need to sort this list so that we don't miss the latest config change.
I don't think this depends on any ordering.
> On April 18, 2014, 3:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/TopicConfigManager.scala, lines 121-134
> > <https://reviews.apache.org/r/20471/diff/1/?file=561891#file561891line121>
> >
> > It seems that if there are no new config changes, the last few config changes in the notification path will not be deleted. This can be a bit confusing.
Yes, that is by design. Otherwise you need some background thread to check. This is less confusing. Change notifications can't pile up because if new ones come that will trigger purging.
> On April 18, 2014, 3:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/TopicConfigManager.scala, lines 123-124
> > <https://reviews.apache.org/r/20471/diff/1/?file=561891#file561891line123>
> >
> > This is not needed. In ZK, new sequential nodes always get a higher id whether previous sequential nodes are deleted or not. ZK server maintains enough info to achieve that.
If you are totally sure I will remove that bit.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40772
-----------------------------------------------------------
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Joel Koshy <jj...@gmail.com>.
> On April 18, 2014, 3:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/TopicConfigManager.scala, lines 123-124
> > <https://reviews.apache.org/r/20471/diff/1/?file=561891#file561891line123>
> >
> > This is not needed. In ZK, new sequential nodes always get a higher id whether previous sequential nodes are deleted or not. ZK server maintains enough info to achieve that.
>
> Jay Kreps wrote:
> If you are totally sure I will remove that bit.
>
> Jun Rao wrote:
> [zk: localhost:2181(CONNECTED) 2] create -s /a 1
> Created /a0000000001
> [zk: localhost:2181(CONNECTED) 3] delete /a0000000001
> [zk: localhost:2181(CONNECTED) 4] create -s /a 2
> Created /a0000000003
> [zk: localhost:2181(CONNECTED) 5] ls /a
> []
> [zk: localhost:2181(CONNECTED) 6] ls /
> [a0000000003, zookeeper]
>
Since we were wondering about this: the non-consecutive ids are because it is based on the parent's cVersion (which is incremented when you do the delete)
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40772
-----------------------------------------------------------
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Jun Rao <ju...@gmail.com>.
> On April 18, 2014, 3:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/TopicConfigManager.scala, lines 123-124
> > <https://reviews.apache.org/r/20471/diff/1/?file=561891#file561891line123>
> >
> > This is not needed. In ZK, new sequential nodes always get a higher id whether previous sequential nodes are deleted or not. ZK server maintains enough info to achieve that.
>
> Jay Kreps wrote:
> If you are totally sure I will remove that bit.
[zk: localhost:2181(CONNECTED) 2] create -s /a 1
Created /a0000000001
[zk: localhost:2181(CONNECTED) 3] delete /a0000000001
[zk: localhost:2181(CONNECTED) 4] create -s /a 2
Created /a0000000003
[zk: localhost:2181(CONNECTED) 5] ls /a
[]
[zk: localhost:2181(CONNECTED) 6] ls /
[a0000000003, zookeeper]
> On April 18, 2014, 3:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/TopicConfigManager.scala, lines 87-88
> > <https://reviews.apache.org/r/20471/diff/1/?file=561891#file561891line87>
> >
> > The child list returned by ZK doesn't guarantee any ordering. We will need to sort this list so that we don't miss the latest config change.
>
> Jay Kreps wrote:
> I don't think this depends on any ordering.
Yes, you are right. It doesn't depend on ordering since it always reads from the config path in ZK.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40772
-----------------------------------------------------------
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Jun Rao <ju...@gmail.com>.
> On April 18, 2014, 3:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/TopicConfigManager.scala, lines 97-98
> > <https://reviews.apache.org/r/20471/diff/1/?file=561891#file561891line97>
> >
> > In the corner case, it seems that it's still possible by the time that we try to read this ZK path, it's deleted by another broker. We probably need to handle the ZKNodeNotExistException.
Ok, this is handled in readDataMaybeNull. So this is fine.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40772
-----------------------------------------------------------
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40772
-----------------------------------------------------------
Could you add some comments at the beginning of TopicConfigManager to make it clear that the TopicConfigChange command directly updates the topic config path in ZK and the notification path is just for refreshing the topic config cache in each broker? This wasn't very clear to me after reading the current comment.
core/src/main/scala/kafka/server/TopicConfigManager.scala
<https://reviews.apache.org/r/20471/#comment73897>
The child list returned by ZK doesn't guarantee any ordering. We will need to sort this list so that we don't miss the latest config change.
core/src/main/scala/kafka/server/TopicConfigManager.scala
<https://reviews.apache.org/r/20471/#comment73896>
changeZnode is unused.
core/src/main/scala/kafka/server/TopicConfigManager.scala
<https://reviews.apache.org/r/20471/#comment73899>
In the corner case, it seems that it's still possible by the time that we try to read this ZK path, it's deleted by another broker. We probably need to handle the ZKNodeNotExistException.
core/src/main/scala/kafka/server/TopicConfigManager.scala
<https://reviews.apache.org/r/20471/#comment73900>
It seems that if there are no new config changes, the last few config changes in the notification path will not be deleted. This can be a bit confusing.
core/src/main/scala/kafka/server/TopicConfigManager.scala
<https://reviews.apache.org/r/20471/#comment73898>
This is not needed. In ZK, new sequential nodes always get a higher id whether previous sequential nodes are deleted or not. ZK server maintains enough info to achieve that.
core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala
<https://reviews.apache.org/r/20471/#comment73901>
Missing Apache header.
- Jun Rao
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Joel Koshy <jj...@gmail.com>.
> On April 18, 2014, 6:12 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/TopicConfigManager.scala, lines 87-88
> > <https://reviews.apache.org/r/20471/diff/1/?file=561891#file561891line87>
> >
> > Actually that's not the right example - it would work fine since you only want the later change.
> >
> > How about this:
> > Say you first override retention to x hours.
> > (change 1)
> >
> > Then you override compaction policy (change 2)
> >
> > If you bring up a broker that has been down for some time (and has that topic) then it could fail to apply change 1 if it sees change 2 first.
Ok nm - the topic's config would have been written in zookeeper already. I'll cherry-pick this and the follow-up patch into 0.8.1
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40792
-----------------------------------------------------------
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40792
-----------------------------------------------------------
core/src/main/scala/kafka/server/TopicConfigManager.scala
<https://reviews.apache.org/r/20471/#comment73945>
Actually that's not the right example - it would work fine since you only want the later change.
How about this:
Say you first override retention to x hours.
(change 1)
Then you override compaction policy (change 2)
If you bring up a broker that has been down for some time (and has that topic) then it could fail to apply change 1 if it sees change 2 first.
- Joel Koshy
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Jay Kreps <bo...@gmail.com>.
> On April 18, 2014, 6:01 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/TopicConfigManager.scala, lines 87-88
> > <https://reviews.apache.org/r/20471/diff/1/?file=561891#file561891line87>
> >
> > Say you override the retention to x hours, realize you made a mistake and then override to y hours. If the order can be mixed then it would be an issue no?
I don't think so--the notification just says read the config for topic X. So two updates will just mean two entries saying the config has changed. The only config value that is stored is the final value.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40786
-----------------------------------------------------------
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40786
-----------------------------------------------------------
core/src/main/scala/kafka/server/TopicConfigManager.scala
<https://reviews.apache.org/r/20471/#comment73938>
Say you override the retention to x hours, realize you made a mistake and then override to y hours. If the order can be mixed then it would be an issue no?
- Joel Koshy
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Jay Kreps <bo...@gmail.com>.
> On April 18, 2014, 5:25 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/TopicConfigManager.scala, lines 121-134
> > <https://reviews.apache.org/r/20471/diff/1/?file=561891#file561891line121>
> >
> > You mean that the purge is only triggered by a broker bounce or a new config change? I think that is fine right? i.e., in favor of some separate trigger such as a scheduled task doing that?
> >
> > We could store the last executed change as part of the topic config itself for informational purposes (so people can be sure that a config change still sitting in zookeeper was in fact applied to the topic config).
Yes, though that "ack" would have to be per-broker, and we would need to clean it up if/when the partition moves to another broker.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40780
-----------------------------------------------------------
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: Patch for KAFKA-1398
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40780
-----------------------------------------------------------
core/src/main/scala/kafka/server/TopicConfigManager.scala
<https://reviews.apache.org/r/20471/#comment73918>
This is done in readDataMaybeNull
core/src/main/scala/kafka/server/TopicConfigManager.scala
<https://reviews.apache.org/r/20471/#comment73921>
You mean that the purge is only triggered by a broker bounce or a new config change? I think that is fine right? i.e., in favor of some separate trigger such as a scheduled task doing that?
We could store the last executed change as part of the topic config itself for informational purposes (so people can be sure that a config change still sitting in zookeeper was in fact applied to the topic config).
- Joel Koshy
On April 18, 2014, 12:36 a.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 12:36 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 dynamic config changes are broken.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala d41fd33d91406dfa2ce8c1e1b04a078e983ccadd
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: KAFKA-1398: Follow-up comments on dynamic config
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40809
-----------------------------------------------------------
Ship it!
Ship It!
- Joel Koshy
On April 18, 2014, 8:03 p.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 8:03 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 Dynamic config follow-on-comments.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala 4a4274eef790ff1fd2dfbbd85d44722688bfadee
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala 5a1d5cc36033d0427c6c680d198c02207e10326c
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: KAFKA-1398: Follow-up comments on dynamic config
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/#review40810
-----------------------------------------------------------
Ship it!
Ship It!
- Jun Rao
On April 18, 2014, 8:03 p.m., Jay Kreps wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20471/
> -----------------------------------------------------------
>
> (Updated April 18, 2014, 8:03 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1398
> https://issues.apache.org/jira/browse/KAFKA-1398
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1398 Dynamic config follow-on-comments.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/server/TopicConfigManager.scala 4a4274eef790ff1fd2dfbbd85d44722688bfadee
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala 5a1d5cc36033d0427c6c680d198c02207e10326c
>
> Diff: https://reviews.apache.org/r/20471/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jay Kreps
>
>
Re: Review Request 20471: KAFKA-1398: Follow-up comments on dynamic config
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20471/
-----------------------------------------------------------
(Updated April 18, 2014, 8:03 p.m.)
Review request for kafka.
Summary (updated)
-----------------
KAFKA-1398: Follow-up comments on dynamic config
Bugs: KAFKA-1398
https://issues.apache.org/jira/browse/KAFKA-1398
Repository: kafka
Description (updated)
-------
KAFKA-1398 Dynamic config follow-on-comments.
Diffs (updated)
-----
core/src/main/scala/kafka/server/TopicConfigManager.scala 4a4274eef790ff1fd2dfbbd85d44722688bfadee
core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala 5a1d5cc36033d0427c6c680d198c02207e10326c
Diff: https://reviews.apache.org/r/20471/diff/
Testing
-------
Thanks,
Jay Kreps