Hi Jun, I've made some of the changes:
#1 - was doing this in the leader identification, but not on startup. I've cleaned that up #2 - thoughts on how to word this comment? I'm not sure how to point out not to do something we didn't do :) #3 Fixed #4 I'll need to spend a bunch more time refactoring this than I have right now. The point about the different ports isn't something I considered since our DevOps guys build everything from recipes so they are always the same, but I can see how in a non-cookie cutter world that could happen. #5 Good to know. I've updated the example. However I couldn't reproduce this even with a fairly small fetch buffer so I'm not 100% sure the check is correct. Can you take a look and make sure I don't have an off-by-one error? Or suggest how to make it happen? Thanks, Chris On Wed, Apr 24, 2013 at 12:26 PM, Jun Rao <jun...@gmail.com> wrote: > Chris, > > The following are some comments on the SimpleConsumer wiki. > > 1. PartitionMetadata.leader() can return null if the new leader is not > elected yet. We need to handle that. > 2. When using FetchRequestBuilder, it's important NOT to set replicaId > since this is only meant to be used by fetchers in the follower replicas. > Setting replicaId incorrectly will cause the broker to behave strangely. We > didn't do that in the code, but it would be useful to add a comment to > highlight this. > 3. The following code doesn't match the comment. The comment says resetting > to the last offset, but the code resets to the first offset. > > // We asked for an invalid offset. For simple case > ask for the last element to reset readOffset = > getLastOffset(consumer,a_topic, a_partition, > kafka.api.OffsetRequest.EarliestTime(), clientName); > > 4. It seems that we can combine findNewLeader() and fineLeader() into one > method. In particular, the port of the new leader may not be the same as > the old one. > 5. There is one more surprise when iterating messages in a messageSet > retuned in a fetch response. In general, if the fetch offset is X, you may > get messages with offsets less than X in the returned messageSet. This is > because in 0.8, each message in a compressed unit has its own offset. So, > if a compressed messageSet contains 10 messages with offsets from 0 to 9 > and the client wants to fetch from offset 5, the broker is going to return > all 10 messages. So, it's the responsibility of the client to skip the > first 5 messages with offsets from 0 to 4 and only consume messages with > offset 5 and above. Otherwise, the client may get duplicates. This is > handled in the high level consumer and we need to handle that in > SimpleConsumer as well. > > Thanks, > > Jun > > > > On Fri, Mar 29, 2013 at 8:28 AM, Chris Curtin <curtin.ch...@gmail.com > >wrote: > > > Hi, > > > > I've added an example program for using a SimpleConsumer for 0.8.0. Turns > > out to be a little more complicated once you add Broker failover. I'm not > > 100% thrilled with how I detect and recover, so if someone has a better > way > > of doing this please let me (and this list) know. > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/0.8.0+SimpleConsumer+Example > > > > Thanks, > > > > Chris > > > > > > On Mon, Mar 25, 2013 at 8:02 AM, Chris Curtin <curtin.ch...@gmail.com > > >wrote: > > > > > > > > Hi David, > > > > > > Thanks for the feedback. I've seen the example before and after in > > > different books/articles and it doesn't matter to me. > > > > > > Anyone else want to help define a style guide or is there one I didn't > > see > > > already? > > > > > > Thanks, > > > > > > Chris > > > > > > > > > On Thu, Mar 21, 2013 at 7:46 PM, David Arthur <mum...@gmail.com> > wrote: > > > > > >> This looks great! A few comments > > >> > > >> * I think it would be useful to start with a complete example (ready > to > > >> copy/paste) and then break it down bit by bit > > >> * Some of the formatting is funky (gratuitous newlines), also I think > 2 > > >> spaces looks nicer than 4 > > >> * In the text, it might be useful to embolden or italicize class names > > >> > > >> Also, maybe we should move this to a separate thread? > > >> > > > > > > > > >
