Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Kafka, mail # dev - Re: Review Request 15201: address previous review comments


Copy link to this message
-
Re: Review Request 15201: address previous review comments
Jun Rao 2013-11-11, 16:56


> On Nov. 5, 2013, 6:27 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, line 66
> > <https://reviews.apache.org/r/15201/diff/1/?file=377064#file377064line66>
> >
> >     Default max.message.size is (1MB + header). Default fetch size should be a little larger than 1MB. How about 1.1 MB?

The fetch size is 1024 * 1024 (the default), which is larger than the default message size 1000000.
> On Nov. 5, 2013, 6:27 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, line 145
> > <https://reviews.apache.org/r/15201/diff/1/?file=377064#file377064line145>
> >
> >     This can throw an exception if the leader doesn't exist
> >    
> >     Replicas that don't have a leader when this tool is run cannot set the initial offset. Should we skip such partitions?

The tool assumes all leaders are available when the tool is started. Since this tool is mostly used for testing and we want to verify all replicas, I think this is a reasonable assumption. Once the tool is started, leaders can change or become unavailable.
> On Nov. 5, 2013, 6:27 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, line 245
> > <https://reviews.apache.org/r/15201/diff/1/?file=377064#file377064line245>
> >
> >     It will be useful to know the fetch offset of the chunk that is undergoing verification.

There is another debug logging that logs the offset.
> On Nov. 5, 2013, 6:27 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, line 247
> > <https://reviews.apache.org/r/15201/diff/1/?file=377064#file377064line247>
> >
> >     map{ -> map {

Not sure this is the standard. We do map(, instead of map (. So, it seems that we should do map{, instead of map {?
> On Nov. 5, 2013, 6:27 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, line 274
> > <https://reviews.apache.org/r/15201/diff/1/?file=377064#file377064line274>
> >
> >     What happens if one replica returns empty message set because it is not caught up and other replicas return some valid messages? Then ideally the fetch offset should not be updated since we need to repeat the verification for that fetch offset

For an offset to move, all replicas must have received the message on that offset. Otherwise, the tool just keeps retrying.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15201/#review28186
-----------------------------------------------------------
On Nov. 11, 2013, 4:44 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15201/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2013, 4:44 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1117
>     https://issues.apache.org/jira/browse/KAFKA-1117
>
>
> Repository: kafka
>
>
> Description
> -------
>
> kafka-1117; fix 2
>
>
> kafka-1117; fix 1
>
>
> kafka-1117
>
>
> Diffs
> -----
>
>   core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>