-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33242/
-----------------------------------------------------------
(Updated April 16, 2015, 5:03 p.m.)
Review request for kafka.
Changes
-------
address Ewen's review feedbacks
I'm getting a bunch of checkstyle complaints when I try to test. These should
all be easy to fix (and should be causing tests to fail before even running).
The only rule that might not be obvious from the error message is that the
static final field in MockMetricsReporter is expected to be all-caps since it
looks like a constant to checkstyle.
> fixed
In the constructor, could we throw some subclass of KafkaException instead? The
new clients try to stick to that exception hierarchy except in a few special
cases. Alternatively, maybe if we caught Error and RuntimeException instead of
Throwable then we could just rethrow the same exception?
> I changed RuntimeException to KafkaException. can't think of a good subclass
> name for this scenario. ProducerConstructException? hence, stay with the
> generic KafkaException
The new version of close() will swallow exceptions when called normally (i.e.
not from the constructor). They'll be logged, but the caller won't see the
exception anymore. Maybe we should save the first exception and rethrow it?
> refactored a private close(boolean swallowException) method
Exception messages should be capitalized.
> fixed
In the test, we should probably have an assert outside the catch. And is there
any reason the closeCount is being reset to 0?
> yes. we should have an assert outside the catch
> I was just reset the CLOSE_COUNT in case another test method need to check
> the count.
Bugs: KAFKA-2121
https://issues.apache.org/jira/browse/KAFKA-2121
Repository: kafka
Description (updated)
-------
fix potential resource leak when KafkaProducer throws exception in the middle
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
b91e2c52ed0acb1faa85915097d97bafa28c413a
clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java
PRE-CREATION
Diff: https://reviews.apache.org/r/33242/diff/
Testing
-------
Thanks,
Steven Wu
