You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jun Rao <ju...@gmail.com> on 2013/11/12 17:34:40 UTC
Re: Review Request 15201: address more review comments
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15201/
-----------------------------------------------------------
(Updated Nov. 12, 2013, 4:34 p.m.)
Review request for kafka.
Summary (updated)
-----------------
address more review comments
Bugs: KAFKA-1117
https://issues.apache.org/jira/browse/KAFKA-1117
Repository: kafka
Description (updated)
-------
kafka-1117; fix 3
kafka-1117; fix 2
kafka-1117; fix 1
kafka-1117
Diffs (updated)
-----
config/tools-log4j.properties 79240490149835656e2a013a9702c5aa41c104f1
core/src/main/scala/kafka/api/OffsetResponse.scala 08dc3cd3d166efba6b2b43f6e148f636b175affe
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala PRE-CREATION
Diff: https://reviews.apache.org/r/15201/diff/
Testing
-------
Thanks,
Jun Rao
Re: Review Request 15201: address more review comments
Posted by Jun Rao <ju...@gmail.com>.
> 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:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15201/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2013, 4:34 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1117
> https://issues.apache.org/jira/browse/KAFKA-1117
>
>
> Repository: kafka
>
>
> Description
> -------
>
> kafka-1117; fix 3
>
>
> kafka-1117; fix 2
>
>
> kafka-1117; fix 1
>
>
> kafka-1117
>
>
> Diffs
> -----
>
> config/tools-log4j.properties 79240490149835656e2a013a9702c5aa41c104f1
> core/src/main/scala/kafka/api/OffsetResponse.scala 08dc3cd3d166efba6b2b43f6e148f636b175affe
> core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 15201: address more review comments
Posted by Swapnil Ghike <sg...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15201/#review28781
-----------------------------------------------------------
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment55792>
fetchsize -> fetch-size for consistency with other options?
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment55793>
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.
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment55794>
Should we just do .replaceAll("""["']""", "") instead?
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment55799>
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?
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment55807>
Minor comment: Can we rename this to initialOffsetMap (we use the offsetMap name in ReplicaFetchThread), I got confused on the first glace..
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment55805>
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.
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment55806>
Wondering why we don't exit when checksums don't match?
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment55808>
typo
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment55809>
typo
Another caveat seems to be that the tool cannot handle changes in 1. partition leadership change 2. topic configuration change (number of partitions).
- Swapnil Ghike
On Nov. 12, 2013, 4:34 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15201/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2013, 4:34 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1117
> https://issues.apache.org/jira/browse/KAFKA-1117
>
>
> Repository: kafka
>
>
> Description
> -------
>
> kafka-1117; fix 3
>
>
> kafka-1117; fix 2
>
>
> kafka-1117; fix 1
>
>
> kafka-1117
>
>
> Diffs
> -----
>
> config/tools-log4j.properties 79240490149835656e2a013a9702c5aa41c104f1
> core/src/main/scala/kafka/api/OffsetResponse.scala 08dc3cd3d166efba6b2b43f6e148f636b175affe
> core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 15201: address more review comments
Posted by Jun Rao <ju...@gmail.com>.
> On Nov. 13, 2013, 2:09 a.m., Joel Koshy wrote:
> > Looks good overall - this is definitely a useful tool to have. I'm posting comments so far but I will come back to it tomorrow to finish up.
> >
> > So what would be the recommended procedure when the tool reports an inconsistency? Would operations have to go in, stop the brokers and truncate the log segments to the last segments that are verified to be consistent? If so, it would be convenient if the tool could issue another OffsetRequest to report the offset boundaries of the last consistent segments so you don't have to work too hard to determine the segment file.
> >
> > Also, unless I'm reading it incorrect additional caveats are that the tool can only keep working so long as: partitions are not being added (or reassigned); logs are not being compacted.
> >
The tool is meant for detecting inconsistent replicas during testing so that we can fix the bugs. Fixing inconsistent replicas in operation will be hard and will require manual work.
The limitation of the tools is that it won't pick up newly created partitions or partitions moved by the reassignment tool. For those cases, we can just restart the tool.
> On Nov. 13, 2013, 2:09 a.m., Joel Koshy wrote:
> > config/tools-log4j.properties, line 16
> > <https://reviews.apache.org/r/15201/diff/3/?file=382689#file382689line16>
> >
> > Is this needed?
removed.
> On Nov. 13, 2013, 2:09 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, line 98
> > <https://reviews.apache.org/r/15201/diff/3/?file=382691#file382691line98>
> >
> > This is becoming my most frequent review comment :)
> > Can you use CommandLineUtils.checkRequiredArgs?
done.
> On Nov. 13, 2013, 2:09 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala, line 106
> > <https://reviews.apache.org/r/15201/diff/3/?file=382691#file382691line106>
> >
> > Any reason to not use kafka.consumer.Whitelist and the isTopicAllowed API that it provides?
Yes, it can be reused.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15201/#review28744
-----------------------------------------------------------
On Nov. 12, 2013, 4:34 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15201/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2013, 4:34 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1117
> https://issues.apache.org/jira/browse/KAFKA-1117
>
>
> Repository: kafka
>
>
> Description
> -------
>
> kafka-1117; fix 3
>
>
> kafka-1117; fix 2
>
>
> kafka-1117; fix 1
>
>
> kafka-1117
>
>
> Diffs
> -----
>
> config/tools-log4j.properties 79240490149835656e2a013a9702c5aa41c104f1
> core/src/main/scala/kafka/api/OffsetResponse.scala 08dc3cd3d166efba6b2b43f6e148f636b175affe
> core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 15201: address more review comments
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15201/#review28744
-----------------------------------------------------------
Looks good overall - this is definitely a useful tool to have. I'm posting comments so far but I will come back to it tomorrow to finish up.
So what would be the recommended procedure when the tool reports an inconsistency? Would operations have to go in, stop the brokers and truncate the log segments to the last segments that are verified to be consistent? If so, it would be convenient if the tool could issue another OffsetRequest to report the offset boundaries of the last consistent segments so you don't have to work too hard to determine the segment file.
Also, unless I'm reading it incorrect additional caveats are that the tool can only keep working so long as: partitions are not being added (or reassigned); logs are not being compacted.
config/tools-log4j.properties
<https://reviews.apache.org/r/15201/#comment55727>
Is this needed?
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment55733>
This is becoming my most frequent review comment :)
Can you use CommandLineUtils.checkRequiredArgs?
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment55734>
Any reason to not use kafka.consumer.Whitelist and the isTopicAllowed API that it provides?
- Joel Koshy
On Nov. 12, 2013, 4:34 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15201/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2013, 4:34 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1117
> https://issues.apache.org/jira/browse/KAFKA-1117
>
>
> Repository: kafka
>
>
> Description
> -------
>
> kafka-1117; fix 3
>
>
> kafka-1117; fix 2
>
>
> kafka-1117; fix 1
>
>
> kafka-1117
>
>
> Diffs
> -----
>
> config/tools-log4j.properties 79240490149835656e2a013a9702c5aa41c104f1
> core/src/main/scala/kafka/api/OffsetResponse.scala 08dc3cd3d166efba6b2b43f6e148f636b175affe
> core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 15201: address all review comments
Posted by Swapnil Ghike <sg...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15201/#review28873
-----------------------------------------------------------
Ship it!
Ship It!
- Swapnil Ghike
On Nov. 14, 2013, 4:24 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15201/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2013, 4:24 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1117
> https://issues.apache.org/jira/browse/KAFKA-1117
>
>
> Repository: kafka
>
>
> Description
> -------
>
> kafka-1117; fix 4
>
>
> kafka-1117; fix 3
>
>
> kafka-1117; fix 2
>
>
> kafka-1117; fix 1
>
>
> kafka-1117
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/OffsetResponse.scala 08dc3cd3d166efba6b2b43f6e148f636b175affe
> core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 15201: address Neha's review comments
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15201/#review29067
-----------------------------------------------------------
Ship it!
Ship It!
- Neha Narkhede
On Nov. 18, 2013, 5:57 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15201/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 5:57 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1117
> https://issues.apache.org/jira/browse/KAFKA-1117
>
>
> Repository: kafka
>
>
> Description
> -------
>
> kafka-1117; fix 5
>
>
> kafka-1117; fix 4
>
>
> kafka-1117; fix 3
>
>
> kafka-1117; fix 2
>
>
> kafka-1117; fix 1
>
>
> kafka-1117
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/OffsetResponse.scala 08dc3cd3d166efba6b2b43f6e148f636b175affe
> core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 15201: address Neha's review comments
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15201/
-----------------------------------------------------------
(Updated Nov. 18, 2013, 5:57 p.m.)
Review request for kafka.
Summary (updated)
-----------------
address Neha's review comments
Bugs: KAFKA-1117
https://issues.apache.org/jira/browse/KAFKA-1117
Repository: kafka
Description (updated)
-------
kafka-1117; fix 5
kafka-1117; fix 4
kafka-1117; fix 3
kafka-1117; fix 2
kafka-1117; fix 1
kafka-1117
Diffs (updated)
-----
core/src/main/scala/kafka/api/OffsetResponse.scala 08dc3cd3d166efba6b2b43f6e148f636b175affe
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala PRE-CREATION
Diff: https://reviews.apache.org/r/15201/diff/
Testing
-------
Thanks,
Jun Rao
Re: Review Request 15201: address all review comments
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15201/#review29023
-----------------------------------------------------------
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment56101>
PartitionAndReplica instead of (String,Int,Int)
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment56104>
It seems like the message iterator can have multiple messages, but we verify only the first message per replica?
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment56102>
It seems somewhat awkward that internal data structure of the replica buffer has leaked into the replica fetcher. How about adding an API to put the message set for a TopicAndPartition into the ReplicaBuffer?
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
<https://reviews.apache.org/r/15201/#comment56103>
here is where we could use the ReplicaBuffer's API instead of accessing the message set cache directly
- Neha Narkhede
On Nov. 14, 2013, 4:24 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15201/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2013, 4:24 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1117
> https://issues.apache.org/jira/browse/KAFKA-1117
>
>
> Repository: kafka
>
>
> Description
> -------
>
> kafka-1117; fix 4
>
>
> kafka-1117; fix 3
>
>
> kafka-1117; fix 2
>
>
> kafka-1117; fix 1
>
>
> kafka-1117
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/OffsetResponse.scala 08dc3cd3d166efba6b2b43f6e148f636b175affe
> core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 15201: address all review comments
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15201/#review28872
-----------------------------------------------------------
Ship it!
Ship It!
- Guozhang Wang
On Nov. 14, 2013, 4:24 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15201/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2013, 4:24 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1117
> https://issues.apache.org/jira/browse/KAFKA-1117
>
>
> Repository: kafka
>
>
> Description
> -------
>
> kafka-1117; fix 4
>
>
> kafka-1117; fix 3
>
>
> kafka-1117; fix 2
>
>
> kafka-1117; fix 1
>
>
> kafka-1117
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/OffsetResponse.scala 08dc3cd3d166efba6b2b43f6e148f636b175affe
> core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/15201/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 15201: address all review comments
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15201/
-----------------------------------------------------------
(Updated Nov. 14, 2013, 4:24 p.m.)
Review request for kafka.
Summary (updated)
-----------------
address all review comments
Bugs: KAFKA-1117
https://issues.apache.org/jira/browse/KAFKA-1117
Repository: kafka
Description (updated)
-------
kafka-1117; fix 4
kafka-1117; fix 3
kafka-1117; fix 2
kafka-1117; fix 1
kafka-1117
Diffs (updated)
-----
core/src/main/scala/kafka/api/OffsetResponse.scala 08dc3cd3d166efba6b2b43f6e148f636b175affe
core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala PRE-CREATION
Diff: https://reviews.apache.org/r/15201/diff/
Testing
-------
Thanks,
Jun Rao