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 more review comments


Copy link to this message
-
Re: Review Request 15201: address more review comments


> On Nov. 13, 2013, 7:43 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, line 70
> > <https://reviews.apache.org/r/15201/diff/3/?file=382691#file382691line70>
> >
> >     fetchsize -> fetch-size for consistency with other options?

changed.
> On Nov. 13, 2013, 7:43 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, line 74
> > <https://reviews.apache.org/r/15201/diff/3/?file=382691#file382691line74>
> >
> >     We can probably change this to ConsumerConfig.FetchSize. Anytime we change the max message size on the broker, we will probably change default fetch size on consumer, so that can serve as the source of truth for this tool as well.

changed.
> On Nov. 13, 2013, 7:43 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, lines 110-111
> > <https://reviews.apache.org/r/15201/diff/3/?file=382691#file382691line110>
> >
> >     Should we just do .replaceAll("""["']""", "") instead?

Not long used.
> On Nov. 13, 2013, 7:43 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, lines 165-181
> > <https://reviews.apache.org/r/15201/diff/3/?file=382691#file382691line165>
> >
> >     val fetcherThreads = for ((i, element) <- topicAndPartitionsPerBroker.view.zipWithIndex) yield new ReplicaFetcher(...., doVerification = if (i == 0) true else false)
> >    
> >     to avoid variable = true and then variable = false?

Changed, in a different way.
> On Nov. 13, 2013, 7:43 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, lines 242-244
> > <https://reviews.apache.org/r/15201/diff/3/?file=382691#file382691line242>
> >
> >     Minor comment: Can we rename this to initialOffsetMap (we use the offsetMap name in ReplicaFetchThread), I got confused on the first glace..

done.
> On Nov. 13, 2013, 7:43 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, line 272
> > <https://reviews.apache.org/r/15201/diff/3/?file=382691#file382691line272>
> >
> >     I thought this loop is supposed to go through all the messages that can be returned by the messageIterator, but looks like it will check for only the first message obtained via each messageIterator.

This loop is just iterating one message from every replica. The outer loop iterates through all messages.
> On Nov. 13, 2013, 7:43 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, lines 291-295
> > <https://reviews.apache.org/r/15201/diff/3/?file=382691#file382691line291>
> >
> >     Wondering why we don't exit when checksums don't match?

It's probably better to report all occurrences of mismatched checksums.
> On Nov. 13, 2013, 7:43 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, line 376
> > <https://reviews.apache.org/r/15201/diff/3/?file=382691#file382691line376>
> >
> >     typo

changed.
> On Nov. 13, 2013, 7:43 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, line 385
> > <https://reviews.apache.org/r/15201/diff/3/?file=382691#file382691line385>
> >
> >     typo

changed.
On Nov. 13, 2013, 7:43 a.m., Jun Rao wrote:
> > Another caveat seems to be that the tool cannot handle changes in 1. partition leadership change 2. topic configuration change (number of partitions).

The tool doesn't care about leader changes since it has to read from every replica. It won't pick up newly created partitions or partitions moved by the reassignment tool. For those cases, we can just restart the tool.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15201/#review28781
-----------------------------------------------------------
On Nov. 12, 2013, 4:34 p.m., Jun Rao wrote:
>
> ------------------------