> On Nov. 25, 2014, 12:26 a.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/consumer/ConsumerIterator.scala, line 30 > > <https://reviews.apache.org/r/28121/diff/1/?file=765888#file765888line30> > > > > Since we're changing the behavior of ConsumerIterator, let's document > > it here. For example, hasNext is now unblocking.
I'll update the docs since we now have some more methods available. But just to be clear, hasNext is still blocking -- it calls maybeComputeNext() (no params) which calls makeNext() (no params), which calls makeNext(-1, Milliseconds), which implies indefinite timeout. matches the behavior implied by the Iterator interface. I figured that even if we wanted to make it non-blocking, that'd wait for a major version upgrade since that's pretty fundamental behavior that people probably depend on (and certainly aren't catching whatever exception we would throw). > 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? 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 smaller pie ces. But I think breaking it down too much obfuscates the code and isn't really worth the minor deduplication. - 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 > >
