You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Joe Stein <cr...@gmail.com> on 2014/02/10 02:22:00 UTC
Re: Review Request 16718: Patch for KAFKA-1180
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/
-----------------------------------------------------------
(Updated Feb. 10, 2014, 1:21 a.m.)
Review request for kafka.
Bugs: KAFKA-1180
https://issues.apache.org/jira/browse/KAFKA-1180
Repository: kafka
Description
-------
KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex
Diffs (updated)
-----
core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
Diff: https://reviews.apache.org/r/16718/diff/
Testing
-------
Thanks,
Joe Stein
Re: Review Request 16718: Patch for KAFKA-1180
Posted by Joe Stein <cr...@gmail.com>.
> On Feb. 10, 2014, 10:58 p.m., Guozhang Wang wrote:
> > core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala, line 45
> > <https://reviews.apache.org/r/16718/diff/2/?file=481264#file481264line45>
> >
> > Just caught my eye: this does not test anything right?
It was like that before, let me add some more tests on it so it does something moving forward, yup
- Joe
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/#review34128
-----------------------------------------------------------
On Feb. 10, 2014, 1:21 a.m., Joe Stein wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16718/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2014, 1:21 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1180
> https://issues.apache.org/jira/browse/KAFKA-1180
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
> core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
> core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
>
> Diff: https://reviews.apache.org/r/16718/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joe Stein
>
>
Re: Review Request 16718: Patch for KAFKA-1180
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/#review34128
-----------------------------------------------------------
Ship it!
core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala
<https://reviews.apache.org/r/16718/#comment64108>
Just caught my eye: this does not test anything right?
- Guozhang Wang
On Feb. 10, 2014, 1:21 a.m., Joe Stein wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16718/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2014, 1:21 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1180
> https://issues.apache.org/jira/browse/KAFKA-1180
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
> core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
> core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
>
> Diff: https://reviews.apache.org/r/16718/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joe Stein
>
>
Re: Review Request 16718: Patch for KAFKA-1180
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/#review47480
-----------------------------------------------------------
Ship it!
Looks good apart from the minor comment I had on the tests.
- Joel Koshy
On Feb. 13, 2014, 8:24 p.m., Joe Stein wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16718/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2014, 8:24 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1180
> https://issues.apache.org/jira/browse/KAFKA-1180
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex and added Blacklist test that was missing
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
> core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
> core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
>
> Diff: https://reviews.apache.org/r/16718/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joe Stein
>
>
Re: Review Request 16718: Patch for KAFKA-1180
Posted by Joe Stein <cr...@gmail.com>.
> On Feb. 13, 2014, 10:54 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/utils/Utils.scala, line 545
> > <https://reviews.apache.org/r/16718/diff/6/?file=484571#file484571line545>
> >
> > Thanks for the comment. However, I still don't get it. Why are JSON strings
> > relevant here? i.e., we are ultimately dealing with a Java regex in the
> > TopicFilter. I'm sure your patch works (as verified by Jason), but I can't
> > say I understand this.
> >
It is because the regex is used from zookeeper after it is saved as JSON and parsed as JSON for use. e.g. during rebalance from the TopicCount.constructTopicCount
If we set Whitelist("test-(?!bad\\b)[\\w]+") and then consume, when consumer starts it has the regex correct and creates the PartitionTopicInfo for the topic "test-good" which we expect (with an instantiated chunkQueue: BlockingQueue[FetchedDataChunk] for the topic "test-good" and filters out the "test-bad" topic properly)).
The regex patern is then stored in Zookeeper as JSON. From there on the regex is not correct when retrieved and as parsed. So it is missing from the partitionMap in ConsumerFetcherThread (correctly so) but it is still in the topicAndPartition of processPartitionData in ConsumerFetcherThread (because of this issue).
So you get the scenario of the ConsumerFetcherThread trying to enque partitionData for the topic "test-bad" which it doesn't have an instantiated chunkQueue: BlockingQueue[FetchedDataChunk] for the PartitionTopicInfo (nor should it).
> On Feb. 13, 2014, 10:54 p.m., Joel Koshy wrote:
> > core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala, line 51
> > <https://reviews.apache.org/r/16718/diff/6/?file=484572#file484572line51>
> >
> > Do we actually need this test? i.e., my thinking was that we could just add
> > the escaped cases to the whitelist/blacklist tests above - including the
> > specific failure case that Jason reported.
> >
I thought it made sense since the issue was use of this function is where the problem was stemming from
- Joe
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/#review34433
-----------------------------------------------------------
On Feb. 13, 2014, 8:24 p.m., Joe Stein wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16718/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2014, 8:24 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1180
> https://issues.apache.org/jira/browse/KAFKA-1180
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex and added Blacklist test that was missing
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
> core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
> core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
>
> Diff: https://reviews.apache.org/r/16718/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joe Stein
>
>
Re: Review Request 16718: Patch for KAFKA-1180
Posted by Joel Koshy <jj...@gmail.com>.
> On Feb. 13, 2014, 10:54 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/utils/Utils.scala, line 545
> > <https://reviews.apache.org/r/16718/diff/6/?file=484571#file484571line545>
> >
> > Thanks for the comment. However, I still don't get it. Why are JSON strings
> > relevant here? i.e., we are ultimately dealing with a Java regex in the
> > TopicFilter. I'm sure your patch works (as verified by Jason), but I can't
> > say I understand this.
> >
>
> Joe Stein wrote:
> It is because the regex is used from zookeeper after it is saved as JSON and parsed as JSON for use. e.g. during rebalance from the TopicCount.constructTopicCount
>
> If we set Whitelist("test-(?!bad\\b)[\\w]+") and then consume, when consumer starts it has the regex correct and creates the PartitionTopicInfo for the topic "test-good" which we expect (with an instantiated chunkQueue: BlockingQueue[FetchedDataChunk] for the topic "test-good" and filters out the "test-bad" topic properly)).
>
> The regex patern is then stored in Zookeeper as JSON. From there on the regex is not correct when retrieved and as parsed. So it is missing from the partitionMap in ConsumerFetcherThread (correctly so) but it is still in the topicAndPartition of processPartitionData in ConsumerFetcherThread (because of this issue).
>
> So you get the scenario of the ConsumerFetcherThread trying to enque partitionData for the topic "test-bad" which it doesn't have an instantiated chunkQueue: BlockingQueue[FetchedDataChunk] for the PartitionTopicInfo (nor should it).
>
Got it - thanks for the explanation.
> On Feb. 13, 2014, 10:54 p.m., Joel Koshy wrote:
> > core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala, line 51
> > <https://reviews.apache.org/r/16718/diff/6/?file=484572#file484572line51>
> >
> > Do we actually need this test? i.e., my thinking was that we could just add
> > the escaped cases to the whitelist/blacklist tests above - including the
> > specific failure case that Jason reported.
> >
>
> Joe Stein wrote:
> I thought it made sense since the issue was use of this function is where the problem was stemming from
True - although I felt it may be clearer to (implicitly) exercise that logic through real whitelist/blacklist tests such as the one that was reported in the ticket.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/#review34433
-----------------------------------------------------------
On Feb. 13, 2014, 8:24 p.m., Joe Stein wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16718/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2014, 8:24 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1180
> https://issues.apache.org/jira/browse/KAFKA-1180
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex and added Blacklist test that was missing
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
> core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
> core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
>
> Diff: https://reviews.apache.org/r/16718/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joe Stein
>
>
Re: Review Request 16718: Patch for KAFKA-1180
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/#review34433
-----------------------------------------------------------
core/src/main/scala/kafka/utils/Utils.scala
<https://reviews.apache.org/r/16718/#comment64525>
Thanks for the comment. However, I still don't get it. Why are JSON strings
relevant here? i.e., we are ultimately dealing with a Java regex in the
TopicFilter. I'm sure your patch works (as verified by Jason), but I can't
say I understand this.
core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala
<https://reviews.apache.org/r/16718/#comment64526>
Do we actually need this test? i.e., my thinking was that we could just add
the escaped cases to the whitelist/blacklist tests above - including the
specific failure case that Jason reported.
- Joel Koshy
On Feb. 13, 2014, 8:24 p.m., Joe Stein wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16718/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2014, 8:24 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1180
> https://issues.apache.org/jira/browse/KAFKA-1180
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex and added Blacklist test that was missing
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
> core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
> core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
>
> Diff: https://reviews.apache.org/r/16718/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joe Stein
>
>
Re: Review Request 16718: Patch for KAFKA-1180
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/#review34422
-----------------------------------------------------------
Ship it!
Ship It!
- Guozhang Wang
On Feb. 13, 2014, 8:24 p.m., Joe Stein wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16718/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2014, 8:24 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1180
> https://issues.apache.org/jira/browse/KAFKA-1180
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex and added Blacklist test that was missing
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
> core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
> core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
>
> Diff: https://reviews.apache.org/r/16718/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joe Stein
>
>
Re: Review Request 16718: Patch for KAFKA-1180
Posted by Joe Stein <cr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/
-----------------------------------------------------------
(Updated Feb. 13, 2014, 8:24 p.m.)
Review request for kafka.
Bugs: KAFKA-1180
https://issues.apache.org/jira/browse/KAFKA-1180
Repository: kafka
Description
-------
KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex and added Blacklist test that was missing
Diffs (updated)
-----
core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
Diff: https://reviews.apache.org/r/16718/diff/
Testing
-------
Thanks,
Joe Stein
Re: Review Request 16718: Patch for KAFKA-1180
Posted by Joe Stein <cr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/
-----------------------------------------------------------
(Updated Feb. 13, 2014, 8:21 p.m.)
Review request for kafka.
Bugs: KAFKA-1180
https://issues.apache.org/jira/browse/KAFKA-1180
Repository: kafka
Description (updated)
-------
KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex and added Blacklist test that was missing
Diffs (updated)
-----
README.md e3fea22fe103dc690eef4956d535a4f3987b778c
core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
Diff: https://reviews.apache.org/r/16718/diff/
Testing
-------
Thanks,
Joe Stein
Re: Review Request 16718: Patch for KAFKA-1180
Posted by Joe Stein <cr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/
-----------------------------------------------------------
(Updated Feb. 13, 2014, 8:16 p.m.)
Review request for kafka.
Bugs: KAFKA-1180
https://issues.apache.org/jira/browse/KAFKA-1180
Repository: kafka
Description
-------
KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex and added Blacklist test that was missing
KAFKA-1263 Snazzy up the README markdown for better visibility on github
Diffs
-----
README.md e3fea22fe103dc690eef4956d535a4f3987b778c
core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
Diff: https://reviews.apache.org/r/16718/diff/
Testing
-------
Thanks,
Joe Stein
Re: Review Request 16718: Patch for KAFKA-1180
Posted by Joe Stein <cr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/
-----------------------------------------------------------
(Updated Feb. 13, 2014, 8:13 p.m.)
Review request for kafka.
Bugs: KAFKA-1180
https://issues.apache.org/jira/browse/KAFKA-1180
Repository: kafka
Description (updated)
-------
KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex and added Blacklist test that was missing
KAFKA-1263 Snazzy up the README markdown for better visibility on github
Diffs (updated)
-----
README.md e3fea22fe103dc690eef4956d535a4f3987b778c
core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
Diff: https://reviews.apache.org/r/16718/diff/
Testing
-------
Thanks,
Joe Stein
Re: Review Request 16718: Patch for KAFKA-1180
Posted by Joe Stein <cr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16718/
-----------------------------------------------------------
(Updated Feb. 13, 2014, 7:50 p.m.)
Review request for kafka.
Bugs: KAFKA-1180
https://issues.apache.org/jira/browse/KAFKA-1180
Repository: kafka
Description (updated)
-------
KAFKA-1180 added a test for blacklist that was missing
KAFKA-1180 WhiteList topic filter gets a NullPointerException on complex Regex
KAFKA-1263 Snazzy up the README markdown for better visibility on github
Diffs (updated)
-----
README.md e3fea22fe103dc690eef4956d535a4f3987b778c
core/src/main/scala/kafka/consumer/TopicCount.scala e33263378489f7cb5ba98476a8e6d65640130965
core/src/main/scala/kafka/utils/Utils.scala a89b0463685e6224d263bc9177075e1bb6b93d04
core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala cf2724bb68d39256f033687c25cde24c667c3d8d
Diff: https://reviews.apache.org/r/16718/diff/
Testing
-------
Thanks,
Joe Stein