You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Grant Henke <gr...@gmail.com> on 2015/03/24 16:51:06 UTC
Review Request 32440: Patch for KAFKA-2043
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32440/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-2043
https://issues.apache.org/jira/browse/KAFKA-2043
Repository: kafka
Description
-------
CompressionType is passed in each RecordAccumulator append
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java feda9c922d7dab17e424f8e6f0aa0a3f968e3729
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 88b4e4fbf3bf6fb6d2f90551a792b95d4cd51c40
clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java e379ac89c9a2fbfe750d6b0dec693b7eabb76204
clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java 24274a64885fadd0e9318de2beb232218ddd52cd
Diff: https://reviews.apache.org/r/32440/diff/
Testing
-------
Thanks,
Grant Henke
Re: Review Request 32440: Patch for KAFKA-2043
Posted by Grant Henke <gr...@gmail.com>.
> On March 24, 2015, 5 p.m., Mayuresh Gharat wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 134
> > <https://reviews.apache.org/r/32440/diff/1/?file=904417#file904417line134>
> >
> > Since its a Producer level config, is this change needed. We can keep it as an instance variable. Also since the compression type does not change, the "private final" makes it more clear. What do you think?
I don't mind leaving the instance level config. However, since it is not used anywhere but the constructor I don't see the value in it. If we want to mark it as final we can in the constructor and have the same clarity. The only reason I didn't initially is because the other code did not seam to follow the style of putting final on everything. (Note: I would prefer to put final on everything)
- Grant
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32440/#review77593
-----------------------------------------------------------
On March 24, 2015, 3:51 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32440/
> -----------------------------------------------------------
>
> (Updated March 24, 2015, 3:51 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2043
> https://issues.apache.org/jira/browse/KAFKA-2043
>
>
> Repository: kafka
>
>
> Description
> -------
>
> CompressionType is passed in each RecordAccumulator append
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java feda9c922d7dab17e424f8e6f0aa0a3f968e3729
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 88b4e4fbf3bf6fb6d2f90551a792b95d4cd51c40
> clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java e379ac89c9a2fbfe750d6b0dec693b7eabb76204
> clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java 24274a64885fadd0e9318de2beb232218ddd52cd
>
> Diff: https://reviews.apache.org/r/32440/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 32440: Patch for KAFKA-2043
Posted by Mayuresh Gharat <gh...@gmail.com>.
> On March 24, 2015, 5 p.m., Mayuresh Gharat wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 134
> > <https://reviews.apache.org/r/32440/diff/1/?file=904417#file904417line134>
> >
> > Since its a Producer level config, is this change needed. We can keep it as an instance variable. Also since the compression type does not change, the "private final" makes it more clear. What do you think?
>
> Grant Henke wrote:
> I don't mind leaving the instance level config. However, since it is not used anywhere but the constructor I don't see the value in it. If we want to mark it as final we can in the constructor and have the same clarity. The only reason I didn't initially is because the other code did not seam to follow the style of putting final on everything. (Note: I would prefer to put final on everything)
Completely agree that its not used as much. Not having it as instance level config is not going to effect the functionality, just that someone would prefer the configs to be together.
- Mayuresh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32440/#review77593
-----------------------------------------------------------
On March 24, 2015, 3:51 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32440/
> -----------------------------------------------------------
>
> (Updated March 24, 2015, 3:51 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2043
> https://issues.apache.org/jira/browse/KAFKA-2043
>
>
> Repository: kafka
>
>
> Description
> -------
>
> CompressionType is passed in each RecordAccumulator append
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java feda9c922d7dab17e424f8e6f0aa0a3f968e3729
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 88b4e4fbf3bf6fb6d2f90551a792b95d4cd51c40
> clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java e379ac89c9a2fbfe750d6b0dec693b7eabb76204
> clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java 24274a64885fadd0e9318de2beb232218ddd52cd
>
> Diff: https://reviews.apache.org/r/32440/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 32440: Patch for KAFKA-2043
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32440/#review77593
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
<https://reviews.apache.org/r/32440/#comment125718>
Since its a Producer level config, is this change needed. We can keep it as an instance variable. Also since the compression type does not change, the "private final" makes it more clear. What do you think?
- Mayuresh Gharat
On March 24, 2015, 3:51 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32440/
> -----------------------------------------------------------
>
> (Updated March 24, 2015, 3:51 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2043
> https://issues.apache.org/jira/browse/KAFKA-2043
>
>
> Repository: kafka
>
>
> Description
> -------
>
> CompressionType is passed in each RecordAccumulator append
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java feda9c922d7dab17e424f8e6f0aa0a3f968e3729
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 88b4e4fbf3bf6fb6d2f90551a792b95d4cd51c40
> clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java e379ac89c9a2fbfe750d6b0dec693b7eabb76204
> clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java 24274a64885fadd0e9318de2beb232218ddd52cd
>
> Diff: https://reviews.apache.org/r/32440/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 32440: Patch for KAFKA-2043
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32440/#review77787
-----------------------------------------------------------
Ship it!
- Mayuresh Gharat
On March 25, 2015, 6:29 p.m., Grant Henke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32440/
> -----------------------------------------------------------
>
> (Updated March 25, 2015, 6:29 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2043
> https://issues.apache.org/jira/browse/KAFKA-2043
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Promote compressionType to class variable
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java ab263423ff1d33170effb71acdef3fc501fa072a
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 88b4e4fbf3bf6fb6d2f90551a792b95d4cd51c40
> clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java e379ac89c9a2fbfe750d6b0dec693b7eabb76204
> clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java 24274a64885fadd0e9318de2beb232218ddd52cd
>
> Diff: https://reviews.apache.org/r/32440/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Grant Henke
>
>
Re: Review Request 32440: Patch for KAFKA-2043
Posted by Grant Henke <gr...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32440/
-----------------------------------------------------------
(Updated March 25, 2015, 6:29 p.m.)
Review request for kafka.
Bugs: KAFKA-2043
https://issues.apache.org/jira/browse/KAFKA-2043
Repository: kafka
Description (updated)
-------
Promote compressionType to class variable
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java ab263423ff1d33170effb71acdef3fc501fa072a
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 88b4e4fbf3bf6fb6d2f90551a792b95d4cd51c40
clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java e379ac89c9a2fbfe750d6b0dec693b7eabb76204
clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java 24274a64885fadd0e9318de2beb232218ddd52cd
Diff: https://reviews.apache.org/r/32440/diff/
Testing
-------
Thanks,
Grant Henke