Re: Review Request 36346: fix heartbeat to allow offset commits during PreparingRebalance
On July 9, 2015, 4:58 p.m., Ewen Cheslack-Postava wrote: Just FYI, this was also noticed in the patch for KAFKA-2123 (which is moving quite a bit of code around). The solution was a bit different (the condition is different and keeps the existing generation ID increment location). Thanks for the heads up Ewen. I started looking at that patch yesterday but didn't reach the changes to core (it's at the end of the rb). It looks like Jason roughly followed option 1 from my comment in https://issues.apache.org/jira/browse/KAFKA-1740?focusedCommentId=14620038page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14620038 . I prefer the approach I provided because the generation id increment logic is easier to understand and the logged state transitions with their generation ids are more readable. - Onur --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36346/#review91118 --- On July 9, 2015, 9:42 a.m., Onur Karaman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36346/ --- (Updated July 9, 2015, 9:42 a.m.) Review request for kafka. Bugs: KAFKA-1740 https://issues.apache.org/jira/browse/KAFKA-1740 Repository: kafka Description --- fix heartbeat to allow offset commits during PreparingRebalance Diffs - core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala 476973b2c551db5be3f1c54f94990f0dd15ff65e Diff: https://reviews.apache.org/r/36346/diff/ Testing --- Manual testing only so far. The OffsetManager merge didn't add anything to ConsumerCoordinatorResponseTest.scala. I'll try to add all the missing OffsetCommitRequest and OffsetFetchRequest handling tests to this rb. Thanks, Onur Karaman
Re: Review Request 36346: fix heartbeat to allow offset commits during PreparingRebalance
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36346/ --- (Updated July 9, 2015, 9:42 a.m.) Review request for kafka. Bugs: KAFKA-1740 https://issues.apache.org/jira/browse/KAFKA-1740 Repository: kafka Description --- fix heartbeat to allow offset commits during PreparingRebalance Diffs - core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala 476973b2c551db5be3f1c54f94990f0dd15ff65e Diff: https://reviews.apache.org/r/36346/diff/ Testing (updated) --- Manual testing only so far. The OffsetManager merge didn't add anything to ConsumerCoordinatorResponseTest.scala. I'll try to add all the missing OffsetCommitRequest and OffsetFetchRequest handling tests to this rb. Thanks, Onur Karaman
Re: Review Request 36346: fix heartbeat to allow offset commits during PreparingRebalance
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36346/#review91118 --- Just FYI, this was also noticed in the patch for KAFKA-2123 (which is moving quite a bit of code around). The solution was a bit different (the condition is different and keeps the existing generation ID increment location). - Ewen Cheslack-Postava On July 9, 2015, 9:42 a.m., Onur Karaman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36346/ --- (Updated July 9, 2015, 9:42 a.m.) Review request for kafka. Bugs: KAFKA-1740 https://issues.apache.org/jira/browse/KAFKA-1740 Repository: kafka Description --- fix heartbeat to allow offset commits during PreparingRebalance Diffs - core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala 476973b2c551db5be3f1c54f94990f0dd15ff65e Diff: https://reviews.apache.org/r/36346/diff/ Testing --- Manual testing only so far. The OffsetManager merge didn't add anything to ConsumerCoordinatorResponseTest.scala. I'll try to add all the missing OffsetCommitRequest and OffsetFetchRequest handling tests to this rb. Thanks, Onur Karaman
