Re: Review Request 30196: Patch for KAFKA-1886
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/#review82408 --- Ship it! Ship It! - Neha Narkhede On April 28, 2015, 5:28 p.m., Aditya Auradkar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- (Updated April 28, 2015, 5:28 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description --- Fixing KAFKA-1886 Diffs - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 Diff: https://reviews.apache.org/r/30196/diff/ Testing --- Thanks, Aditya Auradkar
Re: Review Request 30196: Patch for KAFKA-1886
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- (Updated April 28, 2015, 5:28 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description --- Fixing KAFKA-1886 Diffs - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 Diff: https://reviews.apache.org/r/30196/diff/ Testing (updated) --- Thanks, Aditya Auradkar
Re: Review Request 30196: Patch for KAFKA-1886
On Feb. 7, 2015, 4:22 p.m., Neha Narkhede wrote: core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala, line 295 https://reviews.apache.org/r/30196/diff/2/?file=844610#file844610line295 Why do you need the sleep here? We try to avoid blindly sleeping in Kafka tests since it almost always leads to transient test failures. Consider using TestUtils.waitUntilTrue(). Aditya Auradkar wrote: Thanks Neha. I missed this review comment. I agree sleeping isn't ideal here but I don't think there is a condition I can wait on to trigger this specific exception. The client has to be waiting on a response from the server. I'm not even sure that this testcase needs to exist in PrimitiveApiTest since it isn't testing an API. Can you suggest a better place to put it, if it makes sense to keep it at all? Neha Narkhede wrote: Yeah, if we can't make a fullproof unit test, let's remove it. We are really trying to reduce the number of randomly failing unit tests. Hey Neha, I've removed the test. - Aditya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/#review71556 --- On April 28, 2015, 5:28 p.m., Aditya Auradkar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- (Updated April 28, 2015, 5:28 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description --- Fixing KAFKA-1886 Diffs - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 Diff: https://reviews.apache.org/r/30196/diff/ Testing --- Thanks, Aditya Auradkar
Re: Review Request 30196: Patch for KAFKA-1886
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- (Updated April 28, 2015, 5:27 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description (updated) --- Fixing KAFKA-1886 Diffs (updated) - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 Diff: https://reviews.apache.org/r/30196/diff/ Testing --- Added an integration test to PrimitiveAPITest.scala. Thanks, Aditya Auradkar
Re: Review Request 30196: Patch for KAFKA-1886
On Feb. 7, 2015, 4:22 p.m., Neha Narkhede wrote: core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala, line 295 https://reviews.apache.org/r/30196/diff/2/?file=844610#file844610line295 Why do you need the sleep here? We try to avoid blindly sleeping in Kafka tests since it almost always leads to transient test failures. Consider using TestUtils.waitUntilTrue(). Aditya Auradkar wrote: Thanks Neha. I missed this review comment. I agree sleeping isn't ideal here but I don't think there is a condition I can wait on to trigger this specific exception. The client has to be waiting on a response from the server. I'm not even sure that this testcase needs to exist in PrimitiveApiTest since it isn't testing an API. Can you suggest a better place to put it, if it makes sense to keep it at all? Yeah, if we can't make a fullproof unit test, let's remove it. We are really trying to reduce the number of randomly failing unit tests. - Neha --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/#review71556 --- On Feb. 2, 2015, 9:57 p.m., Aditya Auradkar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- (Updated Feb. 2, 2015, 9:57 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description --- Fixing KAFKA-1886. SimpleConsumer should not swallow ClosedByInterruptException Diffs - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala aeb7a19acaefabcc161c2ee6144a56d9a8999a81 Diff: https://reviews.apache.org/r/30196/diff/ Testing --- Added an integration test to PrimitiveAPITest.scala. Thanks, Aditya Auradkar
Re: Review Request 30196: Patch for KAFKA-1886
On Feb. 7, 2015, 4:22 p.m., Neha Narkhede wrote: core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala, line 295 https://reviews.apache.org/r/30196/diff/2/?file=844610#file844610line295 Why do you need the sleep here? We try to avoid blindly sleeping in Kafka tests since it almost always leads to transient test failures. Consider using TestUtils.waitUntilTrue(). Thanks Neha. I missed this review comment. I agree sleeping isn't ideal here but I don't think there is a condition I can wait on to trigger this specific exception. The client has to be waiting on a response from the server. I'm not even sure that this testcase needs to exist in PrimitiveApiTest since it isn't testing an API. Can you suggest a better place to put it, if it makes sense to keep it at all? - Aditya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/#review71556 --- On Feb. 2, 2015, 9:57 p.m., Aditya Auradkar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- (Updated Feb. 2, 2015, 9:57 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description --- Fixing KAFKA-1886. SimpleConsumer should not swallow ClosedByInterruptException Diffs - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala aeb7a19acaefabcc161c2ee6144a56d9a8999a81 Diff: https://reviews.apache.org/r/30196/diff/ Testing --- Added an integration test to PrimitiveAPITest.scala. Thanks, Aditya Auradkar
Re: Review Request 30196: Patch for KAFKA-1886
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/#review71556 --- core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala https://reviews.apache.org/r/30196/#comment117311 Why do you need the sleep here? We try to avoid blindly sleeping in Kafka tests since it almost always leads to transient test failures. Consider using TestUtils.waitUntilTrue(). - Neha Narkhede On Feb. 2, 2015, 9:57 p.m., Aditya Auradkar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- (Updated Feb. 2, 2015, 9:57 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description --- Fixing KAFKA-1886. SimpleConsumer should not swallow ClosedByInterruptException Diffs - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala aeb7a19acaefabcc161c2ee6144a56d9a8999a81 Diff: https://reviews.apache.org/r/30196/diff/ Testing --- Added an integration test to PrimitiveAPITest.scala. Thanks, Aditya Auradkar
Re: Review Request 30196: Patch for KAFKA-1886
On Jan. 26, 2015, 1:28 a.m., Neha Narkhede wrote: core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala, line 235 https://reviews.apache.org/r/30196/diff/1/?file=831148#file831148line235 what is the purpose of this sleep? Aditya Auradkar wrote: I wanted to make sure the SimpleConsumer was making a request to the broker when I interrupted. I can reduce the sleep time to 100ms if that helps. Neha, Is this the right place to add such a test case. I've added it to PrimitiveApiTest but this isn't really testing an API.. merely testing SimpleConsumer behavior. - Aditya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/#review69579 --- On Feb. 2, 2015, 9:57 p.m., Aditya Auradkar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- (Updated Feb. 2, 2015, 9:57 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description --- Fixing KAFKA-1886. SimpleConsumer should not swallow ClosedByInterruptException Diffs - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala aeb7a19acaefabcc161c2ee6144a56d9a8999a81 Diff: https://reviews.apache.org/r/30196/diff/ Testing --- Added an integration test to PrimitiveAPITest.scala. Thanks, Aditya Auradkar
Re: Review Request 30196: Patch for KAFKA-1886
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- (Updated Feb. 2, 2015, 9:57 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description (updated) --- Fixing KAFKA-1886. SimpleConsumer should not swallow ClosedByInterruptException Diffs (updated) - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala aeb7a19acaefabcc161c2ee6144a56d9a8999a81 Diff: https://reviews.apache.org/r/30196/diff/ Testing --- Added an integration test to PrimitiveAPITest.scala. Thanks, Aditya Auradkar
Re: Review Request 30196: Patch for KAFKA-1886
On Jan. 26, 2015, 1:28 a.m., Neha Narkhede wrote: core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala, line 235 https://reviews.apache.org/r/30196/diff/1/?file=831148#file831148line235 what is the purpose of this sleep? I wanted to make sure the SimpleConsumer was making a request to the broker when I interrupted. I can reduce the sleep time to 100ms if that helps. - Aditya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/#review69579 --- On Jan. 22, 2015, 10:35 p.m., Aditya Auradkar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- (Updated Jan. 22, 2015, 10:35 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description --- Fixing KAFKA-1886. Forcing the SimpleConsumer to throw a ClosedByInterruptException if thrown and not retry Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk Diffs - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala a5386a03b62956bc440b40783247c8cdf7432315 Diff: https://reviews.apache.org/r/30196/diff/ Testing --- Added an integration test to PrimitiveAPITest.scala. Thanks, Aditya Auradkar
Re: Review Request 30196: Patch for KAFKA-1886
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/#review69579 --- core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala https://reviews.apache.org/r/30196/#comment114287 what is the purpose of this sleep? - Neha Narkhede On Jan. 22, 2015, 10:35 p.m., Aditya Auradkar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- (Updated Jan. 22, 2015, 10:35 p.m.) Review request for kafka and Joel Koshy. Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description --- Fixing KAFKA-1886. Forcing the SimpleConsumer to throw a ClosedByInterruptException if thrown and not retry Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk Diffs - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala a5386a03b62956bc440b40783247c8cdf7432315 Diff: https://reviews.apache.org/r/30196/diff/ Testing --- Added an integration test to PrimitiveAPITest.scala. Thanks, Aditya Auradkar
Review Request 30196: Patch for KAFKA-1886
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- Review request for kafka. Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description --- Fixing KAFKA-1886. Forcing the SimpleConsumer to throw a ClosedByInterruptException if thrown and not retry Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk Diffs - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala a5386a03b62956bc440b40783247c8cdf7432315 Diff: https://reviews.apache.org/r/30196/diff/ Testing --- Thanks, Aditya Auradkar
Re: Review Request 30196: Patch for KAFKA-1886
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30196/ --- (Updated Jan. 22, 2015, 10:35 p.m.) Review request for kafka and Joel Koshy. Changes --- + Added joel to review Bugs: KAFKA-1886 https://issues.apache.org/jira/browse/KAFKA-1886 Repository: kafka Description --- Fixing KAFKA-1886. Forcing the SimpleConsumer to throw a ClosedByInterruptException if thrown and not retry Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk Diffs - core/src/main/scala/kafka/consumer/SimpleConsumer.scala cbef84ac76e62768981f74e71d451f2bda995275 core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala a5386a03b62956bc440b40783247c8cdf7432315 Diff: https://reviews.apache.org/r/30196/diff/ Testing (updated) --- Added an integration test to PrimitiveAPITest.scala. Thanks, Aditya Auradkar
