You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Manikumar Reddy O <ma...@gmail.com> on 2014/07/21 17:16:37 UTC
Re: Review Request 23705: Addressing Jun's comments
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23705/
-----------------------------------------------------------
(Updated July 21, 2014, 3:16 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Addressing Jun's comments
Bugs: KAFKA-1192
https://issues.apache.org/jira/browse/KAFKA-1192
Repository: kafka
Description (updated)
-------
Support given for custom deserialization of messages and keys
Diffs (updated)
-----
core/src/main/scala/kafka/tools/DumpLogSegments.scala 6daf87b25a48a51aafb7dbe8d0c0371e0ea7501f
Diff: https://reviews.apache.org/r/23705/diff/
Testing
-------
Thanks,
Manikumar Reddy O
Re: Review Request 23705: Addressing Jun's comments
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23705/
-----------------------------------------------------------
(Updated July 23, 2014, 2:55 p.m.)
Review request for kafka.
Bugs: KAFKA-1192
https://issues.apache.org/jira/browse/KAFKA-1192
Repository: kafka
Description
-------
Support given for custom deserialization of messages and keys
Diffs (updated)
-----
core/src/main/scala/kafka/tools/DumpLogSegments.scala 6daf87b25a48a51aafb7dbe8d0c0371e0ea7501f
Diff: https://reviews.apache.org/r/23705/diff/
Testing
-------
Thanks,
Manikumar Reddy O
Re: Review Request 23705: Addressing Jun's comments
Posted by Manikumar Reddy O <ma...@gmail.com>.
> On July 21, 2014, 6:16 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/tools/DumpLogSegments.scala, line 165
> > <https://reviews.apache.org/r/23705/diff/3/?file=636778#file636778line165>
> >
> > Would key also be null possibly?
>
> Manikumar Reddy O wrote:
> This line is surrounded by if(msg.hasKey) check.
> msg.hasKey returns true for non-zero length keys. So we don't need any additional null check.
>
>
> Guozhang Wang wrote:
> I see. Instead of not printing anything if key is null, could you still print " key: null" to be aligned with other keyed messages. This will also make text editing for analysis like grep/awk easy.
We need to use "if(msg.hasKey)" to differentiate keyed messages with non-keyed messages.
Key will be null/zero length for all non-keyed messages. So it is not appropriate to print " key: null" for all non-keyed messages.
If "msg.hasKey" true means it is a keyed-message, So it key will not be null.
Are you saying that there can be a message with non-zero key length but with null reference?
- Manikumar Reddy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23705/#review48265
-----------------------------------------------------------
On July 23, 2014, 2:55 p.m., Manikumar Reddy O wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23705/
> -----------------------------------------------------------
>
> (Updated July 23, 2014, 2:55 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1192
> https://issues.apache.org/jira/browse/KAFKA-1192
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Support given for custom deserialization of messages and keys
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/DumpLogSegments.scala 6daf87b25a48a51aafb7dbe8d0c0371e0ea7501f
>
> Diff: https://reviews.apache.org/r/23705/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Manikumar Reddy O
>
>
Re: Review Request 23705: Addressing Jun's comments
Posted by Manikumar Reddy O <ma...@gmail.com>.
> On July 21, 2014, 6:16 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/tools/DumpLogSegments.scala, line 165
> > <https://reviews.apache.org/r/23705/diff/3/?file=636778#file636778line165>
> >
> > Would key also be null possibly?
This line is surrounded by if(msg.hasKey) check.
msg.hasKey returns true for non-zero length keys. So we don't need any additional null check.
- Manikumar Reddy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23705/#review48265
-----------------------------------------------------------
On July 21, 2014, 3:16 p.m., Manikumar Reddy O wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23705/
> -----------------------------------------------------------
>
> (Updated July 21, 2014, 3:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1192
> https://issues.apache.org/jira/browse/KAFKA-1192
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Support given for custom deserialization of messages and keys
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/DumpLogSegments.scala 6daf87b25a48a51aafb7dbe8d0c0371e0ea7501f
>
> Diff: https://reviews.apache.org/r/23705/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Manikumar Reddy O
>
>
Re: Review Request 23705: Addressing Jun's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
> On July 21, 2014, 6:16 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/tools/DumpLogSegments.scala, line 165
> > <https://reviews.apache.org/r/23705/diff/3/?file=636778#file636778line165>
> >
> > Would key also be null possibly?
>
> Manikumar Reddy O wrote:
> This line is surrounded by if(msg.hasKey) check.
> msg.hasKey returns true for non-zero length keys. So we don't need any additional null check.
>
I see. Instead of not printing anything if key is null, could you still print " key: null" to be aligned with other keyed messages. This will also make text editing for analysis like grep/awk easy.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23705/#review48265
-----------------------------------------------------------
On July 21, 2014, 3:16 p.m., Manikumar Reddy O wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23705/
> -----------------------------------------------------------
>
> (Updated July 21, 2014, 3:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1192
> https://issues.apache.org/jira/browse/KAFKA-1192
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Support given for custom deserialization of messages and keys
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/DumpLogSegments.scala 6daf87b25a48a51aafb7dbe8d0c0371e0ea7501f
>
> Diff: https://reviews.apache.org/r/23705/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Manikumar Reddy O
>
>
Re: Review Request 23705: Addressing Jun's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
> On July 21, 2014, 6:16 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/tools/DumpLogSegments.scala, line 165
> > <https://reviews.apache.org/r/23705/diff/3/?file=636778#file636778line165>
> >
> > Would key also be null possibly?
>
> Manikumar Reddy O wrote:
> This line is surrounded by if(msg.hasKey) check.
> msg.hasKey returns true for non-zero length keys. So we don't need any additional null check.
>
>
> Guozhang Wang wrote:
> I see. Instead of not printing anything if key is null, could you still print " key: null" to be aligned with other keyed messages. This will also make text editing for analysis like grep/awk easy.
>
> Manikumar Reddy O wrote:
> We need to use "if(msg.hasKey)" to differentiate keyed messages with non-keyed messages.
>
> Key will be null/zero length for all non-keyed messages. So it is not appropriate to print " key: null" for all non-keyed messages.
>
> If "msg.hasKey" true means it is a keyed-message, So it key will not be null.
>
> Are you saying that there can be a message with non-zero key length but with null reference?
>
>
>
>
>
>
Any keyed messages' key should be not null, what I originally was suggesting is that we can still print "key: null" for non-keyed messages.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23705/#review48265
-----------------------------------------------------------
On July 23, 2014, 2:55 p.m., Manikumar Reddy O wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23705/
> -----------------------------------------------------------
>
> (Updated July 23, 2014, 2:55 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1192
> https://issues.apache.org/jira/browse/KAFKA-1192
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Support given for custom deserialization of messages and keys
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/DumpLogSegments.scala 6daf87b25a48a51aafb7dbe8d0c0371e0ea7501f
>
> Diff: https://reviews.apache.org/r/23705/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Manikumar Reddy O
>
>
Re: Review Request 23705: Addressing Jun's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23705/#review48265
-----------------------------------------------------------
core/src/main/scala/kafka/tools/DumpLogSegments.scala
<https://reviews.apache.org/r/23705/#comment84652>
Would key also be null possibly?
- Guozhang Wang
On July 21, 2014, 3:16 p.m., Manikumar Reddy O wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23705/
> -----------------------------------------------------------
>
> (Updated July 21, 2014, 3:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1192
> https://issues.apache.org/jira/browse/KAFKA-1192
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Support given for custom deserialization of messages and keys
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/DumpLogSegments.scala 6daf87b25a48a51aafb7dbe8d0c0371e0ea7501f
>
> Diff: https://reviews.apache.org/r/23705/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Manikumar Reddy O
>
>
Re: Review Request 23705: Addressing Jun's comments
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23705/#review48440
-----------------------------------------------------------
Looks good. Just one minor comment below.
core/src/main/scala/kafka/tools/DumpLogSegments.scala
<https://reviews.apache.org/r/23705/#comment85043>
It's probably better to call this valueDecoder.
- Jun Rao
On July 21, 2014, 3:16 p.m., Manikumar Reddy O wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23705/
> -----------------------------------------------------------
>
> (Updated July 21, 2014, 3:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1192
> https://issues.apache.org/jira/browse/KAFKA-1192
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Support given for custom deserialization of messages and keys
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/tools/DumpLogSegments.scala 6daf87b25a48a51aafb7dbe8d0c0371e0ea7501f
>
> Diff: https://reviews.apache.org/r/23705/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Manikumar Reddy O
>
>