> On Nov. 25, 2014, 12:26 a.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/utils/IteratorTemplate.scala, line 46 > > <https://reviews.apache.org/r/28121/diff/1/?file=765889#file765889line46> > > > > The change is larger than I expected it to be. What is the reason we > > couldn't change the peek() implementation and add poll to the > > IteratorTemplate? Since IteratorTemplate is not used elsewhere, it may be > > an ok change. > > > > But I guess you must've thought about this and chose not to do it this > > way. Mind explaining the reasoning? > > > > I also think this is the right way to do things. However, weighing that > > with the fact that we are making a somewhat large change to an API that is > > going away, how bad is implementing peek by passing in a consumer timeout > > of 0? > > Ewen Cheslack-Postava wrote: > IteratorTemplate actually is used a bunch of other places via anonymous > classes, although admittedly not publicly. I initially started down the path > of only modifying IteratorTemplate, but it ends up being a bigger, awkward > change since the ConsumerIterator was the only implementation that could > support non-blocking operations. Things get even messier if you support both > peek and poll since now instead of a boolean you have to support a timeout. > Poll is important to support if you want to accurately hit timeouts when you > want to collect more than one message (e.g. a proxy). You'll notice the bulk > of the new non-test code is in NonBlockingIteratorTemplate.poll() because it > doesn't decompose nicely into the existing hasNext() and next() calls, which > means the bulk of the patch would still be there anyway. One possibility to > tighten it up would be to break the original steps into checkNext, getItem, > and clearItem, then implement hasNext, next, peek, and poll using those even > small er pieces. But I think breaking it down too much obfuscates the code and isn't really worth the minor deduplication. > > Neha Narkhede wrote: > Makes sense. I'd imagine the checkNext refactoring won't buy us much. > Curious what you think about the consumer timeout=0 hack?
That's probably a workable solution in a lot of cases -- even for the proxy example since you're always going to be somewhat imprecise and the network is going to make it much more imprecise. But I think it's much harder to work with since now you have to figure out what a reasonable timeout is. 1ms to try to hit the target timeout? Or does that add a lot of overhead if you have a slow stream of data? Seems easier to just do it the right way :) - Ewen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28121/#review62909 ----------------------------------------------------------- On Nov. 25, 2014, 1:24 a.m., Ewen Cheslack-Postava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28121/ > ----------------------------------------------------------- > > (Updated Nov. 25, 2014, 1:24 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1780 > https://issues.apache.org/jira/browse/KAFKA-1780 > > > Repository: kafka > > > Description > ------- > > Expand documentation on ConsumerIterator to reflect new non-blocking APIs. > > > Diffs > ----- > > core/src/main/scala/kafka/consumer/ConsumerIterator.scala > 78fbf75651583e390258af2d9f09df6911a97b59 > core/src/main/scala/kafka/utils/IteratorTemplate.scala > fd952f3ec0f04a3ba639c02779634265489fd186 > core/src/test/scala/unit/kafka/consumer/ConsumerIteratorTest.scala > c0355cc0135c6af2e346b4715659353a31723b86 > core/src/test/scala/unit/kafka/utils/IteratorTemplateTest.scala > 46a4e899ef293c56a931bfa5bcf9a07d07ec5792 > > Diff: https://reviews.apache.org/r/28121/diff/ > > > Testing > ------- > > > Thanks, > > Ewen Cheslack-Postava > >
