----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23568/#review48261 -----------------------------------------------------------
core/src/main/scala/kafka/admin/TopicCommand.scala <https://reviews.apache.org/r/23568/#comment84637> Will need to modify this error message. core/src/main/scala/kafka/controller/ControllerChannelManager.scala <https://reviews.apache.org/r/23568/#comment84639> See comment below in KafkaApis core/src/main/scala/kafka/message/Message.scala <https://reviews.apache.org/r/23568/#comment84645> Should also store the txcontrol in the message header. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/23568/#comment84646> Initially, I was thinking we could just append to local log (since we definitely want to avoid duplicating code) until we have the API for durable append (to a replicated log). That is part of refactoring KafkaApis and is actually blocked on KAFKA-1333 so unless you need this for working through all the failure cases I would suggest just doing a local append for now. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/23568/#comment84647> Looking at this method in the other patch - this only gives the head - what about the other partitions? core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/23568/#comment84648> Rather than do this one partition at a time we should group them by broker. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/23568/#comment84649> I think it is fine to use a channel manager similar to the controller channel manager but that is no longer specific to the controller. i.e., we should probably move it out to become a more generic re-usable "ChannelManager" module. In fact, given the critical nature of controller to broker communication we should probably dedicate a separate channel manager entirely to transactions so that it doesn't interfere with the controller-broker communication. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/23568/#comment84650> Same comments here apply as the above (wrt duplicate code) core/src/main/scala/kafka/server/KafkaConfig.scala <https://reviews.apache.org/r/23568/#comment84654> Should remove the comment on "atomic" commits - that was only for the consumer offsets topic. core/src/main/scala/kafka/server/TransactionManager.scala <https://reviews.apache.org/r/23568/#comment84658> We generally avoid using tuples and use case classes instead - since that is a lot clearer. core/src/main/scala/kafka/server/TransactionManager.scala <https://reviews.apache.org/r/23568/#comment84655> Incorrect comment. core/src/main/scala/kafka/server/TransactionManager.scala <https://reviews.apache.org/r/23568/#comment84667> How about we come up some other name for this - or even just TransactionalHW but that is a bit too wordy. Just want to avoid confusion with the replica HW. core/src/main/scala/kafka/server/TransactionManager.scala <https://reviews.apache.org/r/23568/#comment84661> One issue with this approach is that every commit/abort will cause a linear scan of this queue - we can discuss some alternative ways to maintain the set of pending transactions and associated txcontrol offsets. core/src/main/scala/kafka/server/TransactionManager.scala <https://reviews.apache.org/r/23568/#comment84662> As discussed elsewhere, txid should not be the key. - Joel Koshy On July 18, 2014, 3:12 a.m., Dong Lin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23568/ > ----------------------------------------------------------- > > (Updated July 18, 2014, 3:12 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1523 > https://issues.apache.org/jira/browse/KAFKA-1523 > > > Repository: kafka > > > Description > ------- > > KAFKA-1523 transaction manager module (version 2) > > > Diffs > ----- > > core/src/main/scala/kafka/admin/TopicCommand.scala > 8d5c2e7088fc6e8bf69e775ea7f5893b94580fdf > core/src/main/scala/kafka/common/Topic.scala > ad759786d1c22f67c47808c0b8f227eb2b1a9aa8 > core/src/main/scala/kafka/controller/ControllerChannelManager.scala > 8763968fbff697e4c5c98ab1274627c192a4d26a > core/src/main/scala/kafka/message/Message.scala > d2a7293c7be4022af30884330924791340acc5c1 > core/src/main/scala/kafka/server/KafkaApis.scala > 0b668f230c8556fdf08654ce522a11847d0bf39b > core/src/main/scala/kafka/server/KafkaConfig.scala > ef75b67b67676ae5b8931902cbc8c0c2cc72c0d3 > core/src/main/scala/kafka/server/KafkaServer.scala > c22e51e0412843ec993721ad3230824c0aadd2ba > core/src/main/scala/kafka/server/TransactionManager.scala PRE-CREATION > core/src/main/scala/kafka/utils/ZkUtils.scala > dcdc1ce2b02c996294e19cf480736106aaf29511 > > Diff: https://reviews.apache.org/r/23568/diff/ > > > Testing > ------- > > > Thanks, > > Dong Lin > >
