> 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=14620038&page=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 > >
