Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 21, 2015, 5:48 a.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description (updated) --- move MockMetricsReporter into clients/src/test/java/org/apache/kafka/test Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/ClientUtils.java d0da5d7a08a0c3e67e0fe14bb0b0e7c73380f416 clients/src/main/java/org/apache/kafka/clients/KafkaClient.java 96ac6d0cca990eebe90707465d7d8091c069a4b2 clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 21243345311a106f0802ce96c026ba6e815ccf99 clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 70954cadb5c7e9a4c326afcf9d9a07db230e7db2 clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Deserializer.java 13be6a38cb356d55e25151776328a3c38c573db4 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java PRE-CREATION clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java PRE-CREATION clients/src/test/java/org/apache/kafka/test/MockMetricsReporter.java PRE-CREATION Diff: https://reviews.apache.org/r/33242/diff/ Testing --- Thanks, Steven Wu
Re: Review Request 33242: Patch for KAFKA-2121
On April 21, 2015, 3:08 a.m., Guozhang Wang wrote: LGTM, besides one minor suggestion: could you move MockMetricsReporter to clients/src/test/java/org/apache/kafka/test? done. moved MockMetricsReporter to clients/src/test/java/org/apache/kafka/test - Steven --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80892 --- On April 21, 2015, 5:48 a.m., Steven Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 21, 2015, 5:48 a.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- move MockMetricsReporter into clients/src/test/java/org/apache/kafka/test Diffs - clients/src/main/java/org/apache/kafka/clients/ClientUtils.java d0da5d7a08a0c3e67e0fe14bb0b0e7c73380f416 clients/src/main/java/org/apache/kafka/clients/KafkaClient.java 96ac6d0cca990eebe90707465d7d8091c069a4b2 clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 21243345311a106f0802ce96c026ba6e815ccf99 clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 70954cadb5c7e9a4c326afcf9d9a07db230e7db2 clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Deserializer.java 13be6a38cb356d55e25151776328a3c38c573db4 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java PRE-CREATION clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java PRE-CREATION clients/src/test/java/org/apache/kafka/test/MockMetricsReporter.java PRE-CREATION Diff: https://reviews.apache.org/r/33242/diff/ Testing --- Thanks, Steven Wu
Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80892 --- LGTM, besides one minor suggestion: could you move MockMetricsReporter to clients/src/test/java/org/apache/kafka/test? - Guozhang Wang On April 20, 2015, 4:57 p.m., Steven Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 4:57 p.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- fix potential resource leak when KafkaProducer contructor failed in the middle Diffs - clients/src/main/java/org/apache/kafka/clients/ClientUtils.java d0da5d7a08a0c3e67e0fe14bb0b0e7c73380f416 clients/src/main/java/org/apache/kafka/clients/KafkaClient.java 96ac6d0cca990eebe90707465d7d8091c069a4b2 clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 21243345311a106f0802ce96c026ba6e815ccf99 clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 70954cadb5c7e9a4c326afcf9d9a07db230e7db2 clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Deserializer.java 13be6a38cb356d55e25151776328a3c38c573db4 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java PRE-CREATION 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
Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80770 --- Ship it! New version with KafkaConsumer changes looks good, passes all tests for me. Great work! - Ewen Cheslack-Postava On April 20, 2015, 4:57 p.m., Steven Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 4:57 p.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- fix potential resource leak when KafkaProducer contructor failed in the middle Diffs - clients/src/main/java/org/apache/kafka/clients/ClientUtils.java d0da5d7a08a0c3e67e0fe14bb0b0e7c73380f416 clients/src/main/java/org/apache/kafka/clients/KafkaClient.java 96ac6d0cca990eebe90707465d7d8091c069a4b2 clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 21243345311a106f0802ce96c026ba6e815ccf99 clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 70954cadb5c7e9a4c326afcf9d9a07db230e7db2 clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Deserializer.java 13be6a38cb356d55e25151776328a3c38c573db4 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java PRE-CREATION 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
Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 4:51 p.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- fix potential resource leak when KafkaProducer contructor failed in the middle Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/ClientUtils.java d0da5d7a08a0c3e67e0fe14bb0b0e7c73380f416 clients/src/main/java/org/apache/kafka/clients/KafkaClient.java 96ac6d0cca990eebe90707465d7d8091c069a4b2 clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 21243345311a106f0802ce96c026ba6e815ccf99 clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 70954cadb5c7e9a4c326afcf9d9a07db230e7db2 clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Deserializer.java 13be6a38cb356d55e25151776328a3c38c573db4 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java PRE-CREATION 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
Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 4:57 p.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- fix potential resource leak when KafkaProducer contructor failed in the middle Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/ClientUtils.java d0da5d7a08a0c3e67e0fe14bb0b0e7c73380f416 clients/src/main/java/org/apache/kafka/clients/KafkaClient.java 96ac6d0cca990eebe90707465d7d8091c069a4b2 clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 21243345311a106f0802ce96c026ba6e815ccf99 clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 70954cadb5c7e9a4c326afcf9d9a07db230e7db2 clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Deserializer.java 13be6a38cb356d55e25151776328a3c38c573db4 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java PRE-CREATION 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
Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 4:52 p.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- fix potential resource leak when KafkaProducer contructor failed in the middle Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/ClientUtils.java d0da5d7a08a0c3e67e0fe14bb0b0e7c73380f416 clients/src/main/java/org/apache/kafka/clients/KafkaClient.java 96ac6d0cca990eebe90707465d7d8091c069a4b2 clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 21243345311a106f0802ce96c026ba6e815ccf99 clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 70954cadb5c7e9a4c326afcf9d9a07db230e7db2 clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Deserializer.java 13be6a38cb356d55e25151776328a3c38c573db4 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java PRE-CREATION 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
Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80642 --- Ship it! LGTM! If this gets merged as is, we should file a follow-up issue for the new consumer, which has the same issue. - Ewen Cheslack-Postava On April 19, 2015, 3:09 a.m., Steven Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 19, 2015, 3:09 a.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- fix potential resource leak when KafkaProducer contructor failed in the middle Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab 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
Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 3:08 a.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description (updated) --- applied same fix to KafkaConsumer Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/ClientUtils.java d0da5d7a08a0c3e67e0fe14bb0b0e7c73380f416 clients/src/main/java/org/apache/kafka/clients/KafkaClient.java 96ac6d0cca990eebe90707465d7d8091c069a4b2 clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 21243345311a106f0802ce96c026ba6e815ccf99 clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab 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
Re: Review Request 33242: Patch for KAFKA-2121
On April 19, 2015, 11:11 p.m., Ewen Cheslack-Postava wrote: LGTM! If this gets merged as is, we should file a follow-up issue for the new consumer, which has the same issue. OK. I applied the same fix for new consumer. also updated jira title to reflect the expanded scope. - Steven --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80642 --- On April 20, 2015, 3:30 a.m., Steven Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 3:30 a.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- applied same fix to KafkaConsumer add test for KafkaConsumer Diffs - clients/src/main/java/org/apache/kafka/clients/ClientUtils.java d0da5d7a08a0c3e67e0fe14bb0b0e7c73380f416 clients/src/main/java/org/apache/kafka/clients/KafkaClient.java 96ac6d0cca990eebe90707465d7d8091c069a4b2 clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 21243345311a106f0802ce96c026ba6e815ccf99 clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 70954cadb5c7e9a4c326afcf9d9a07db230e7db2 clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java PRE-CREATION 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
Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 20, 2015, 3:30 a.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description (updated) --- applied same fix to KafkaConsumer add test for KafkaConsumer Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/ClientUtils.java d0da5d7a08a0c3e67e0fe14bb0b0e7c73380f416 clients/src/main/java/org/apache/kafka/clients/KafkaClient.java 96ac6d0cca990eebe90707465d7d8091c069a4b2 clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 21243345311a106f0802ce96c026ba6e815ccf99 clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 70954cadb5c7e9a4c326afcf9d9a07db230e7db2 clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java PRE-CREATION 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
Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 19, 2015, 3:09 a.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description (updated) --- fix potential resource leak when KafkaProducer contructor failed in the middle Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab 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
Re: Review Request 33242: Patch for KAFKA-2121
On April 16, 2015, 5:29 p.m., Ewen Cheslack-Postava wrote: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 548 https://reviews.apache.org/r/33242/diff/2/?file=931792#file931792line548 One idea for making this less verbose and redundant: make all of these classes implement Closeable so we can just write one utility method for trying to close something and catching the exception. Steven Wu wrote: yes. I thought about it. it may break binary compatibility, e.g. Serializer. Sender and Metrics classes are probably only used internally. let me know your thoughts. Ewen Cheslack-Postava wrote: I'm pretty sure it's fine, based on this Changing the direct superclass or the set of direct superinterfaces of a class type will not break compatibility with pre-existing binaries, provided that the total set of superclasses or superinterfaces, respectively, of the class type loses no members. from https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html ok. you could be correct. here is another reference (easier to understand than the jls doc) Expand superinterface set (direct or inherited)- Binary compatible from https://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_Interfaces - Steven --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80346 --- On April 19, 2015, 3:09 a.m., Steven Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 19, 2015, 3:09 a.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- fix potential resource leak when KafkaProducer contructor failed in the middle Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b91e2c52ed0acb1faa85915097d97bafa28c413a clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java b3d3d7c56acb445be16a3fbe00f05eaba659be46 clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java c2fdc23239bd2196cd912c3d121b591f21393eab 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
Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 16, 2015, 4:55 p.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description (updated) --- add a unit test file changes based on Ewen's review feedbacks Diffs (updated) - 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
Re: Review Request 33242: Patch for KAFKA-2121
--- 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
Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 16, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description (updated) --- add a unit test file changes based on Ewen's review feedbacks fix capitalization in error log Diffs (updated) - 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
Re: Review Request 33242: Patch for KAFKA-2121
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80346 --- Looks good, left a few comments. KafkaConsumer suffers from this same problem. Patching that should be pretty much identical -- any chance you could extend this to cover that as well? clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java https://reviews.apache.org/r/33242/#comment130187 This code is all single threaded, is the AtomicReference really necessary here? clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java https://reviews.apache.org/r/33242/#comment130194 Minor, but these messages should be cleaned up -- just needs some capitalization. clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java https://reviews.apache.org/r/33242/#comment130195 One idea for making this less verbose and redundant: make all of these classes implement Closeable so we can just write one utility method for trying to close something and catching the exception. - Ewen Cheslack-Postava On April 16, 2015, 5:03 p.m., Steven Wu wrote: --- 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. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- 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
Re: Review Request 33242: Patch for KAFKA-2121
On April 16, 2015, 5:29 p.m., Ewen Cheslack-Postava wrote: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 531 https://reviews.apache.org/r/33242/diff/2/?file=931792#file931792line531 This code is all single threaded, is the AtomicReference really necessary here? not really necessary. just trying to use the compareAndSet. otherwise, I need to do if(firstException == null) firstException = t. I can certainly change it. let me know. On April 16, 2015, 5:29 p.m., Ewen Cheslack-Postava wrote: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 548 https://reviews.apache.org/r/33242/diff/2/?file=931792#file931792line548 One idea for making this less verbose and redundant: make all of these classes implement Closeable so we can just write one utility method for trying to close something and catching the exception. yes. I thought about it. it may break binary compatibility, e.g. Serializer. Sender and Metrics classes are probably only used internally. let me know your thoughts. - Steven --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80346 --- On April 16, 2015, 5:44 p.m., Steven Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 16, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- add a unit test file changes based on Ewen's review feedbacks fix capitalization in error log 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
Re: Review Request 33242: Patch for KAFKA-2121
On April 16, 2015, 5:29 p.m., Ewen Cheslack-Postava wrote: Looks good, left a few comments. KafkaConsumer suffers from this same problem. Patching that should be pretty much identical -- any chance you could extend this to cover that as well? sure. I can extend this to KafkaConsumer later. - Steven --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80346 --- On April 16, 2015, 5:44 p.m., Steven Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 16, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- add a unit test file changes based on Ewen's review feedbacks fix capitalization in error log 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
Re: Review Request 33242: Patch for KAFKA-2121
On April 16, 2015, 5:29 p.m., Ewen Cheslack-Postava wrote: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, line 548 https://reviews.apache.org/r/33242/diff/2/?file=931792#file931792line548 One idea for making this less verbose and redundant: make all of these classes implement Closeable so we can just write one utility method for trying to close something and catching the exception. Steven Wu wrote: yes. I thought about it. it may break binary compatibility, e.g. Serializer. Sender and Metrics classes are probably only used internally. let me know your thoughts. I'm pretty sure it's fine, based on this Changing the direct superclass or the set of direct superinterfaces of a class type will not break compatibility with pre-existing binaries, provided that the total set of superclasses or superinterfaces, respectively, of the class type loses no members. from https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html - Ewen --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/#review80346 --- On April 16, 2015, 5:44 p.m., Steven Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33242/ --- (Updated April 16, 2015, 5:44 p.m.) Review request for kafka. Bugs: KAFKA-2121 https://issues.apache.org/jira/browse/KAFKA-2121 Repository: kafka Description --- add a unit test file changes based on Ewen's review feedbacks fix capitalization in error log 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
